Bug 80854 - [Qt][Wk2] Assertion Failure and crash on file upload
Summary: [Qt][Wk2] Assertion Failure and crash on file upload
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 79458
  Show dependency treegraph
 
Reported: 2012-03-12 10:25 PDT by Dinu Jacob
Modified: 2012-05-03 23:01 PDT (History)
7 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Dinu Jacob 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
Comment 1 Dinu Jacob 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.
Comment 2 Dinu Jacob 2012-03-12 10:33:46 PDT
Created attachment 131348 [details]
Patch
Comment 3 Simon Hausmann 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.
Comment 4 Dinu Jacob 2012-03-13 07:20:20 PDT
Yes, will work on a qml replacement. But provided this 'bandage' as it was a crash.
Comment 5 Dinu Jacob 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.
Comment 6 Dinu Jacob 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)
Comment 7 Kenneth Rohde Christiansen 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?
Comment 8 Alexander Færøy 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 :-)
Comment 9 Alexander Færøy 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.
Comment 10 Dinu Jacob 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
Comment 11 Dinu Jacob 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.
Comment 12 Dinu Jacob 2012-03-15 09:48:53 PDT
Created attachment 132063 [details]
Patch
Comment 13 Simon Hausmann 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 {
Comment 14 Dinu Jacob 2012-03-16 06:52:20 PDT
Created attachment 132274 [details]
Patch
Comment 15 WebKit Review Bot 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.
Comment 16 WebKit Review Bot 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>
Comment 17 WebKit Review Bot 2012-03-16 09:16:46 PDT
All reviewed patches have been landed.  Closing bug.
Comment 18 Keith Kyzivat 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
Comment 19 Keith Kyzivat 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?
Comment 20 Simon Hausmann 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 ;(