Bug 80854

Summary:

[Qt][Wk2] Assertion Failure and crash on file upload

Product:

WebKit

Reporter:

Dinu Jacob <dinu.jacob>

Component:

WebKit2

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

ahf, hausmann, kamaji, kasthuri.n-s, menard, webkit.review.bot, zoltan

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

Bug Blocks:

79458

Attachments:

Description	Flags
Patch	hausmann: review-
Path with qml component	none
Patch using qml based file picker	none
Patch	hausmann: review+
Patch	none

Dinu Jacob

Reported 2012-03-12 10:25:33 PDT

Using MiniBrowser, 1. Load http://iop4.nokia-boston.com/users/tests/fileupload/fileupload.html 2. Click on "Browser" to upload a file This causes an ASSERT: ASSERT: "!fileDialog" in file Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp, line 386 Attempting to upload a file after fixing the ASSERT resulted in crash: QWidget: Cannot create a QWidget when no GUI is being used Program received signal SIGABRT, Aborted. 0xb7fe2832 in ?? () from /lib/ld-linux.so.2 (gdb) bt #0 0xb7fe2832 in ?? () from /lib/ld-linux.so.2 #1 0xafc9ce71 in raise () from /lib/i386-linux-gnu/libc.so.6 #2 0xafca034e in abort () from /lib/i386-linux-gnu/libc.so.6 #3 0xb0051ceb in qt_message_output (msgType=QtFatalMsg, context=..., buf=0x817b028 "QWidget: Cannot create a QWidget when no GUI is being used") at global/qlogging.cpp:734 #4 0xb005012c in qt_message (msgType=QtFatalMsg, context=..., msg=0xaf8d272c "QWidget: Cannot create a QWidget when no GUI is being used", ap=0xbfffe178 "[\004") at global/qlogging.cpp:131 #5 0xb005039d in QMessageLogger::fatal (this=0xbfffe1c8, msg=0xaf8d272c "QWidget: Cannot create a QWidget when no GUI is being used") at global/qlogging.cpp:334 #6 0xaf4774e9 in QWidgetPrivate::init (this=0x83c4e08, parentWidget=0x0, f=...) at kernel/qwidget.cpp:1115 #7 0xaf476fce in QWidget::QWidget (this=0x83b09a8, dd=..., parent=0x0, f=...) at kernel/qwidget.cpp:1038 #8 0xaf6e3f0f in QDialog::QDialog (this=0x83b09a8, dd=..., parent=0x0, f=...) at dialogs/qdialog.cpp:323 #9 0xaf6e9d46 in QFileDialog::QFileDialog (this=0x83b09a8, parent=0x0, caption=..., directory=..., filter=...) at dialogs/qfiledialog.cpp:339 #10 0xb7b230a6 in QQuickWebViewPrivate::chooseFiles (this=0x80e45b8, listenerRef=0x83d62e8, selectedFileNames=..., type=QtWebPageUIClient::SingleFileSelection) at Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:407

Attachments
Patch (3.05 KB, patch) 2012-03-12 10:33 PDT, Dinu Jacob	hausmann: review-	Details Formatted Diff Diff
Path with qml component (25.82 KB, patch) 2012-03-14 13:53 PDT, Dinu Jacob	no flags	Details Formatted Diff Diff
Patch using qml based file picker (28.59 KB, patch) 2012-03-14 13:59 PDT, Dinu Jacob	no flags	Details Formatted Diff Diff
Patch (28.49 KB, patch) 2012-03-15 09:48 PDT, Dinu Jacob	hausmann: review+	Details Formatted Diff Diff
Patch (28.53 KB, patch) 2012-03-16 06:52 PDT, Dinu Jacob	no flags	Details Formatted Diff Diff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.

Dinu Jacob

Comment 1 2012-03-12 10:27:06 PDT

The ASSERT was caused by uninitialized member fileDialog. The qFatal crash resulted from trying to create QFileDialog from a QGuiApplication instead of a QApplication.

Dinu Jacob

Comment 2 2012-03-12 10:33:46 PDT

Created attachment 131348 [details] Patch

Simon Hausmann

Comment 3 2012-03-13 01:43:37 PDT

Comment on attachment 131348 [details] Patch I think the right solution is to eliminate the use of QFileDialog in WK2 altogether and treat file choosing just like the other dialog requests: Implementable via QML components through the experimental API.

Dinu Jacob

Comment 4 2012-03-13 07:20:20 PDT

Yes, will work on a qml replacement. But provided this 'bandage' as it was a crash.

Dinu Jacob

Comment 5 2012-03-14 13:53:10 PDT

Created attachment 131915 [details] Path with qml component Added support for qml based file picker. This patch supports only single selection.

Dinu Jacob

Comment 6 2012-03-14 13:59:17 PDT

Created attachment 131918 [details] Patch using qml based file picker Added support for qml based file picker. This patch supports only single selection (most the changes should be the qml for multi-selection)

Kenneth Rohde Christiansen

Comment 7 2012-03-15 07:15:36 PDT

Comment on attachment 131918 [details] Patch using qml based file picker View in context: https://bugs.webkit.org/attachment.cgi?id=131918&action=review Not a review, but a few comments. > Source/WebKit2/UIProcess/API/qt/qquickwebview_p.h:280 > + QDeclarativeComponent* filePicker() const; > + void setFilePicker(QDeclarativeComponent*); It is fine that this is an experimental API, as it might make sense to join it with the media capturer in the near future, so you have something like mediaPicker("file/*"); mediaPicker("images/capture"); [or whatever those mimetypes are called] Maybe it makes sense to prepare for this already? > Source/WebKit2/UIProcess/API/qt/tests/qmltests/common/singlefileupload.html:10 > + var name=new String(""); missing space around = > Tools/MiniBrowser/qt/qml/FilePicker.qml:29 > +import Qt.labs.folderlistmodel 1.0 Do we have this available? > Tools/MiniBrowser/qt/qml/FilePicker.qml:203 > +} > + > + > + > + > + what is up with the newlines?

Alexander Færøy

Comment 8 2012-03-15 07:23:40 PDT

Comment on attachment 131918 [details] Patch using qml based file picker View in context: https://bugs.webkit.org/attachment.cgi?id=131918&action=review Why does it contain a black box for the titlebar.png? > Source/WebKit2/ChangeLog:3 > + [Qt][Wk2] Assertion Failure and crash on file upload I don't think this name is descriptive enough. This patch is implementing the API for having a file picker and the bug name should probably reflect this. > Source/WebKit2/ChangeLog:10 > + to set the qml component for file upload triggered by an input file element. Nitpick: QML :-) > Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:78 > + , filePicker(0) Use filePickerDialog for consistency. > Source/WebKit2/UIProcess/qt/QtDialogRunner.cpp:178 > + void accept(const QVariant& path) { emit fileSelected((path.toStringList())); } Why the extra set of parentheses? > Source/WebKit2/UIProcess/qt/QtDialogRunner.cpp:267 > + connect(contextObject, SIGNAL(fileSelected(const QStringList&)), SLOT(onFileSelected(const QStringList&))); No need to use reference-to-const in slots. > Source/WebKit2/UIProcess/qt/QtDialogRunner.h:55 > + QStringList filePath() const { return m_filepath; } Not sure that I like this name to be on singular form. It is returning a QStringList and not just a single QString. > Tools/MiniBrowser/qt/MiniBrowser.qrc:13 > + <file>icons/titlebar.png</file> Nitpick: We try to keep this list sorted. > Tools/MiniBrowser/qt/MiniBrowser.qrc:21 > + <file>qml/FilePicker.qml</file> Again, FilePickerDialog.qml. > Tools/MiniBrowser/qt/qml/FilePicker.qml:15 > + * THIS SOFTWARE IS PROVIDED BY APPLE COMPUTER, INC. ``AS IS'' AND ANY Looks incorrect to me, but we are very bad and this in general. > Tools/MiniBrowser/qt/qml/FilePicker.qml:29 > +import Qt.labs.folderlistmodel 1.0 I am not sure about this at all, but is this safe enough to use and wont add any new dependencies? > Tools/MiniBrowser/qt/qml/FilePicker.qml:31 > +Rectangle { Any reason why you can't make this work with the already implemented Dialog QML type? > Tools/MiniBrowser/qt/qml/FilePicker.qml:32 > + id: root Change this to id: filePickerDialog instead. Root is too ambiguous. > Tools/MiniBrowser/qt/qml/FilePicker.qml:102 > + id: folderList folderListView, maybe? > Tools/MiniBrowser/qt/qml/FilePicker.qml:199 > + Nitpick: Lots of whitespace in the end :-)

Alexander Færøy

Comment 9 2012-03-15 07:26:27 PDT

After having talked with Kenneth, let's stick with the name you already have instead of adding Dialog.

Dinu Jacob

Comment 10 2012-03-15 08:52:04 PDT

(In reply to comment #7) > (From update of attachment 131918 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=131918&action=review > > Not a review, but a few comments. > > > Source/WebKit2/UIProcess/API/qt/qquickwebview_p.h:280 > > + QDeclarativeComponent* filePicker() const; > > + void setFilePicker(QDeclarativeComponent*); > > It is fine that this is an experimental API, as it might make sense to join it with the media capturer in the near future, so you have something like > > mediaPicker("file/*"); > mediaPicker("images/capture"); > > [or whatever those mimetypes are called] > > Maybe it makes sense to prepare for this already? > As per IRC discussion, will keep it as FilePicker and later add a property for the mime types. > > Source/WebKit2/UIProcess/API/qt/tests/qmltests/common/singlefileupload.html:10 > > + var name=new String(""); > > missing space around = > Will fix this. > > Tools/MiniBrowser/qt/qml/FilePicker.qml:29 > > +import Qt.labs.folderlistmodel 1.0 > > Do we have this available? > This is part of QtDeclarative, available as a separate plugin under Qt labs. As per IRC discussion with Simon, ok to use this. > > Tools/MiniBrowser/qt/qml/FilePicker.qml:203 > > +} > > + > > + > > + > > + > > + > > what is up with the newlines? Will remove those

Dinu Jacob

Comment 11 2012-03-15 08:58:36 PDT

(In reply to comment #8) > (From update of attachment 131918 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=131918&action=review > > Why does it contain a black box for the titlebar.png? > > > Source/WebKit2/ChangeLog:3 > > + [Qt][Wk2] Assertion Failure and crash on file upload > > I don't think this name is descriptive enough. This patch is implementing the API for having a file picker and the bug name should probably reflect this. > The bug was logged for the crash reported. The original patch was fixing this but since we need a solution for both QGuiApplications as well as QApplications, this patch to replace the current QFileDialog implementation was implemented. > > Source/WebKit2/ChangeLog:10 > > + to set the qml component for file upload triggered by an input file element. > > Nitpick: QML :-) > Will change that > > Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:78 > > + , filePicker(0) > > Use filePickerDialog for consistency. > > > Source/WebKit2/UIProcess/qt/QtDialogRunner.cpp:178 > > + void accept(const QVariant& path) { emit fileSelected((path.toStringList())); } > > Why the extra set of parentheses? > > > Source/WebKit2/UIProcess/qt/QtDialogRunner.cpp:267 > > + connect(contextObject, SIGNAL(fileSelected(const QStringList&)), SLOT(onFileSelected(const QStringList&))); > > No need to use reference-to-const in slots. > Will fix it. > > Source/WebKit2/UIProcess/qt/QtDialogRunner.h:55 > > + QStringList filePath() const { return m_filepath; } > > Not sure that I like this name to be on singular form. It is returning a QStringList and not just a single QString. > Can change it to filePaths. > > Tools/MiniBrowser/qt/MiniBrowser.qrc:13 > > + <file>icons/titlebar.png</file> > This is used as a border image. > Nitpick: We try to keep this list sorted. > > > Tools/MiniBrowser/qt/MiniBrowser.qrc:21 > > + <file>qml/FilePicker.qml</file> > > Again, FilePickerDialog.qml. > > > Tools/MiniBrowser/qt/qml/FilePicker.qml:15 > > + * THIS SOFTWARE IS PROVIDED BY APPLE COMPUTER, INC. ``AS IS'' AND ANY > > Looks incorrect to me, but we are very bad and this in general. > Will fix it. > > Tools/MiniBrowser/qt/qml/FilePicker.qml:29 > > +import Qt.labs.folderlistmodel 1.0 > > I am not sure about this at all, but is this safe enough to use and wont add any new dependencies? > > > Tools/MiniBrowser/qt/qml/FilePicker.qml:31 > > +Rectangle { > > Any reason why you can't make this work with the already implemented Dialog QML type? > Wanted to provide a separate titlebar. > > Tools/MiniBrowser/qt/qml/FilePicker.qml:32 > > + id: root > > Change this to id: filePickerDialog instead. Root is too ambiguous. > Will fix it. > > Tools/MiniBrowser/qt/qml/FilePicker.qml:102 > > + id: folderList > > folderListView, maybe? > Sure. > > Tools/MiniBrowser/qt/qml/FilePicker.qml:199 > > + > > Nitpick: Lots of whitespace in the end :-) Will remove those.

Dinu Jacob

Comment 12 2012-03-15 09:48:53 PDT

Created attachment 132063 [details] Patch

Simon Hausmann

Comment 13 2012-03-16 04:44:54 PDT

Comment on attachment 132063 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=132063&action=review r=me. The style nitpick and the member variable should be fixed before landing. > Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:383 > + openPanelResultListener = listenerRef; It looks like this member variable could be removed, now that we process the file picking "synchronously". > Source/WebKit2/UIProcess/qt/QtDialogRunner.cpp:171 > + QStringList fileList() const {return m_fileList; } Missing space after opening {

Dinu Jacob

Comment 14 2012-03-16 06:52:20 PDT

Created attachment 132274 [details] Patch

WebKit Review Bot

Comment 15 2012-03-16 07:03:29 PDT

Comment on attachment 132274 [details] Patch Rejecting attachment 132274 [details] from commit-queue. maheshk@webkit.org does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py. - If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags. - If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed). The commit-queue restarts itself every 2 hours. After restart the commit-queue will correctly respect your committer rights.

WebKit Review Bot

Comment 16 2012-03-16 09:16:40 PDT

Comment on attachment 132274 [details] Patch Clearing flags on attachment: 132274 Committed r111012: <http://trac.webkit.org/changeset/111012>

WebKit Review Bot

Comment 17 2012-03-16 09:16:46 PDT

All reviewed patches have been landed. Closing bug.

Keith Kyzivat

Comment 18 2012-05-03 16:46:25 PDT

How exactly does one go about installing Qt.labs.folderlistmodel ? There should be some pointers/instructions for installing this up at the BuildingQtOnLinux page. Right now, if one follows the instructions at: http://trac.webkit.org/wiki/BuildingQtOnLinux, they don't get a functioning MiniBrowser (Tools/Scripts/run-launcher -2 --qt fails to run). Instead they're greeted with a plain white window and (an excerpt of ) the following errors on stdout/sterr: qrc:/qml/BrowserWindow.qml:318:34: Type FilePicker unavailable qrc:/qml/FilePicker.qml:22:1: plugin cannot be loaded for module "Qt.labs.folderlistmodel": Unknown error full stdout/stderr: https://gist.github.com/2590486

Keith Kyzivat

Comment 19 2012-05-03 17:08:53 PDT

(In reply to comment #18) > How exactly does one go about installing Qt.labs.folderlistmodel ? And now I feel slightly dumb.. QML_IMPORT_PATH=<path-to-qt5-build>/qtdeclarative/imports Tools/Scripts/run-launcher -2 --qt Is there a more automatic way to set the QML import path, based on where qmake lives or where the makespecs are, or some other way?

Simon Hausmann

Comment 20 2012-05-03 23:01:09 PDT

There used to be (at least) a silly conflict between QtQuick1 and QtQuick2 having the same QML component, causing a nice clash when you build WebKit against both. Could it be that you have QtQuick1 built and it ends up loading that listmodel instead? If that's the case, then it's a Qt bug ;(

Note You need to log in before you can comment on or make changes to this bug.