Bug 80854 – [Qt][Wk2] Assertion Failure and crash on file upload

• Component: WebKit2
• Status: RESOLVED FIXED

[Dinu Jacob]

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

[Dinu Jacob]

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]

Created attachment 131348 [details]
Patch

[Simon Hausmann]

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]

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

[Dinu Jacob]

Created attachment 131915 [details]
Path with qml component

Added support for qml based file picker. This patch supports only single selection.

[Dinu Jacob]

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 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 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]

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

[Dinu Jacob]

(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]

(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]

Created attachment 132063 [details]
Patch

[Simon Hausmann]

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]

Created attachment 132274 [details]
Patch

[WebKit Review Bot]

Comment on attachment 132274 [details]
Patch

Rejecting attachment 132274 [details] from commit-queue.

[e-mail] 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 on attachment 132274 [details]
Patch

Clearing flags on attachment: 132274

Committed r111012: <http://trac.webkit.org/changeset/111012>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Keith Kyzivat]

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]

(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]

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 ;(