Bug 32723

Summary: [Qt] QWebFrame::setUrl works only from second time if url fragment is present
Product: WebKit Reporter: Rion <rion4ik>
Component: WebKit QtAssignee: Caio Marcelo de Oliveira Filho <cmarcelo>
Status: RESOLVED FIXED    
Severity: Normal CC: andy.nichols, aparna.nand, benjamin, cmarcelo, colbroth, commit-queue, kent.hansen, matt, pva
Priority: P2 Keywords: Qt, QtTriaged
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Linux   
Bug Depends on: 29595, 57865    
Bug Blocks:    
Attachments:
Description Flags
Testcase
none
patch
tonikitoo: review-
autotest for qt api
none
patch v2, all qt api tests pass now
benjamin: review-
idea of how to stress more the setUrl() -- not a real patch yet
none
patch v3, storing the url used in setter
benjamin: review-
patch v4, more autotest coverage
none
patch v5, keep a member with URL none

Description Rion 2009-12-18 10:51:03 PST
if url contains fragment ('#' sign and maybe some text after)
QWebFrame just cleans screen when we do setUrl. And only if we
do setUrl second time with the same url, page becomes visible.

simple testcase which needs double execution:
QUrl url = QUrl::fromLocalFile("/path/to/test.html");
url.setFragment("");
ui->webView->setUrl(url);

bug is reproduced at least on 2 computers with absolutely
different hardware. both have fully updated gentoo ~amd64 system,
qt-4.6.0
Comment 1 Rion 2009-12-18 11:08:04 PST
load method works fine with any url
Comment 2 Tor Arne Vestbø 2010-03-10 06:27:47 PST
Please follow the QtWebKit bug reporting guidelines when reporting bugs.

See http://trac.webkit.org/wiki/QtWebKitBugs

Specifically:

  - The 'QtWebKit' component should only be used for bugs/features in the
    public QtWebKit API layer, not to signify that the bug is specific to
    the Qt port of WebKit

      http://trac.webkit.org/wiki/QtWebKitBugs#Component

  - Add the keyword 'Qt' to signal that it's a Qt-related bug

      http://trac.webkit.org/wiki/QtWebKitBugs#Keywords
Comment 3 Kent Hansen 2010-03-11 04:25:33 PST
Reproduced on Linux against r55658.
Comment 4 Kent Hansen 2010-03-11 04:26:17 PST
Created attachment 50487 [details]
Testcase
Comment 5 pva 2010-09-21 16:32:09 PDT
Note: this breaks kchmviewer application http://www.kchmviewer.net/
Comment 6 Jenya Brodskaia 2010-11-09 08:31:00 PST
Cannot reproduce on Linux for r71415
Comment 7 pva 2010-11-15 04:46:03 PST
Still reproducible in qt-4.7.1. Jenya was that revision supposed to be in 4.7.1?
Comment 8 Jenya Brodskaia 2010-11-15 06:40:05 PST
No, that was on webkit trunk.
Comment 9 Benjamin Poulain 2011-03-01 04:09:00 PST
*** Bug 55370 has been marked as a duplicate of this bug. ***
Comment 10 Caio Marcelo de Oliveira Filho 2011-03-02 12:56:26 PST
Reproduced with Linux 64 bits, webkit trunk r80142 and Qt from 4.7 branch commit b31b4c46e70401745572b808c916088afb839614.

Investigating the problem.
Comment 11 Caio Marcelo de Oliveira Filho 2011-03-02 17:00:57 PST
Created attachment 84489 [details]
patch
Comment 12 Alexis Menard (darktears) 2011-03-02 17:09:03 PST
Comment on attachment 84489 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=84489&action=review

kenneth'd...sorry caio...

> Source/WebKit/qt/ChangeLog:-4
> -- 

why this?
Comment 13 Caio Marcelo de Oliveira Filho 2011-03-02 17:43:27 PST
Comment on attachment 84489 [details]
patch

Alexis, I've checked and the patch is clean, so I'm changing to r? cq? again.

The "remove line" you saw is part of the signature generated by git and not part of the patch contents. See bug 29317 ;-)
Comment 14 Caio Marcelo de Oliveira Filho 2011-03-03 06:05:06 PST
Comment on attachment 84489 [details]
patch

Investigating a possible regression caused by this change in one of the tests.
Comment 15 Antonio Gomes 2011-03-03 06:55:28 PST
Comment on attachment 84489 [details]
patch

Caio, I think it needs tests.

r- due to the lack of tests.
Comment 16 Aparna Nandyal 2011-03-03 06:59:00 PST
Please check https://bugs.webkit.org/show_bug.cgi?id=55370 and https://bugs.webkit.org/show_bug.cgi?id=54380 for further discussion on this problem and a related problem.
Comment 17 Caio Marcelo de Oliveira Filho 2011-03-03 11:59:56 PST
Created attachment 84601 [details]
autotest for qt api

This is not the fix, just the autotest.
Comment 18 WebKit Commit Bot 2011-03-03 20:50:29 PST
Comment on attachment 84601 [details]
autotest for qt api

Clearing flags on attachment: 84601

Committed r80314: <http://trac.webkit.org/changeset/80314>
Comment 19 WebKit Commit Bot 2011-03-03 20:50:34 PST
All reviewed patches have been landed.  Closing bug.
Comment 20 Caio Marcelo de Oliveira Filho 2011-03-04 04:01:51 PST
Reopening since the bug is still there.
Comment 21 Caio Marcelo de Oliveira Filho 2011-03-04 06:30:53 PST
The patch breaks url() because it depended on the value of the URL being stored during the clearance of the frame. I'm testing a new patch that remove this dependency to complement the current patch.
Comment 22 Caio Marcelo de Oliveira Filho 2011-03-06 07:04:16 PST
Created bug 55842 to simplify logic of requestedUrl() and permit we use it for implementing our url() after a setUrl() to an invalid url.

The biggest issue I'm trying to solve here is to not depend on an invalid url be stored in our document loader.
Comment 23 Caio Marcelo de Oliveira Filho 2011-03-11 06:57:11 PST
Created attachment 85471 [details]
patch v2, all qt api tests pass now
Comment 24 Benjamin Poulain 2011-03-14 06:26:42 PDT
Comment on attachment 85471 [details]
patch v2, all qt api tests pass now

This looks good (I like the inline function to clarify what is going on there). But I would like to see a bit more test coverage.

Like what is in the history, what if I call setUrl with an default constructed QUrl(), what is the referer for the next request, etc.
Comment 25 Caio Marcelo de Oliveira Filho 2011-03-14 16:35:22 PDT
Created attachment 85741 [details]
idea of how to stress more the setUrl() -- not a real patch yet

Benjamin, thanks for the review.

For the existing implementation, it seems that empty QUrl() doesn't affect history. The attached new test case describes the existing behavior regarding empty QUrl(). Is this kind of test cases you had in mind?

Do you have any tip on how can I track the referrer for the next request using the Qt API? I tried to "grep" for something similar in current autotests but couldn't find.
Comment 26 Benjamin Poulain 2011-03-15 04:16:28 PDT
Comment on attachment 85741 [details]
idea of how to stress more the setUrl() -- not a real patch yet

View in context: https://bugs.webkit.org/attachment.cgi?id=85741&action=review

You should also test that ::setUrl() and url() are consistent. Especially because of the check for "if (!url.isValid())" in ::url().

> Source/WebKit/qt/tests/qwebframe/tst_qwebframe.cpp:653
> +    void emptyUrls();

The name is not very representative of what you are testing.
Comment 27 Benjamin Poulain 2011-03-15 04:19:30 PDT
(In reply to comment #25)
> Do you have any tip on how can I track the referrer for the next request using the Qt API? I tried to "grep" for something similar in current autotests but couldn't find.

I was thinking of subclassing QNetworkAccessManager, and checking the header of the QNetworkRequest.
Comment 28 Caio Marcelo de Oliveira Filho 2011-03-22 22:01:32 PDT
Writing those test cases, I stepped into bug 29595, which I'm adding here as a dependency, since when fixed will get us more test cases.
Comment 29 Caio Marcelo de Oliveira Filho 2011-03-28 20:04:56 PDT
Created attachment 87252 [details]
patch v3, storing the url used in setter

New version of the patch. After bug 29595 we have more tests to verify the behavior.

This new version of the patch adds another auto test, that breaks previous idea of using requestedUrl as fallback -- since load() calls can alter the requestedUrl() without affecting the current url().

Benjamin, I've tried to get some referrer test working, by simulating a click, but even then couldn't get the information available in QNAM (just inside debug statements after the request). More investigation is needed to create such a test, but it seems to me that this could be in another bug, like "[Qt] Create autotest to verify HTTP Referrer when using setUrl() and load()" or something similar. What do you think?
Comment 30 Benjamin Poulain 2011-03-30 09:05:35 PDT
Comment on attachment 87252 [details]
patch v3, storing the url used in setter

I would like test coverage for (1) I setUrl() twice with the same url. (2) I set the same base url but with a new fragment
If that is already covered by existing test, just r? again and ping me :)
Comment 31 Caio Marcelo de Oliveira Filho 2011-03-30 14:09:54 PDT
Created attachment 87611 [details]
patch v4, more autotest coverage

More autotest coverage.

This commit adds autotests to document current behavior when we set the same URL twice. Currently setUrl() acts more like a "clear(); load()" and less like a typical setter function (that wouldn't do anything if the url to be set is the same as current).
Comment 32 Kenneth Rohde Christiansen 2011-03-30 14:16:39 PDT
Comment on attachment 87611 [details]
patch v4, more autotest coverage

View in context: https://bugs.webkit.org/attachment.cgi?id=87611&action=review

> Source/WebKit/qt/Api/qwebframe.cpp:736
> +static inline void clearFrame(WebCore::Frame* frame)
> +{
> +    frame->loader()->activeDocumentLoader()->writer()->begin();
> +    frame->loader()->activeDocumentLoader()->writer()->end();
> +}

Maybe "clearCoreFrame" ? as this it in in qwebframe.

> Source/WebKit/qt/Api/qwebframe_p.h:115
> +    WebCore::KURL urlFromSetter;

What if I call load? shouldn't this be cleared somehow?

Btw, "...used when THE document URL is not YET available"

> Source/WebKit/qt/tests/qwebframe/tst_qwebframe.cpp:2419
> +void tst_QWebFrame::requestedUrlSetAndLoadFails()

Failed?
Comment 33 Caio Marcelo de Oliveira Filho 2011-03-30 15:06:47 PDT
(In reply to comment #32)
> > Source/WebKit/qt/Api/qwebframe_p.h:115
> > +    WebCore::KURL urlFromSetter;
> 
> What if I call load? shouldn't this be cleared somehow?

Nice catch. Thanks! Not clearing was intentional, but I was not considering this load() case.

I'll work on a different version of the patch to take care of this.
Comment 34 Caio Marcelo de Oliveira Filho 2011-04-05 11:29:50 PDT
Created another bug for the autotests that cover url() and friends but are not related to this patch. Bug 57865 patch contains tests that cover issues pointed by comment 30 and comment 32.
Comment 35 Caio Marcelo de Oliveira Filho 2011-04-06 14:10:49 PDT
Created attachment 88509 [details]
patch v5, keep a member with URL

Use a member to track the URL. Both setUrl() and FrameLoaderClient callbacks will update it, the latter at the same moment that urlChanged() signal is emitted.

Benjamin, rationale for KURL is in the ChangeLog.

Kenneth, previous approach worked even in the case you mentioned (at least as much as I could track in the auto tests created). So no cleanup of urlFromSetter wasn't necessary ;-). However, as we discussed before, the current approach is simpler ("URL is in one well-known place"), and will be less confusing to debug in the future.
Comment 36 Benjamin Poulain 2011-04-07 08:54:02 PDT
Comment on attachment 88509 [details]
patch v5, keep a member with URL

Looks good
Comment 37 WebKit Commit Bot 2011-04-07 10:38:22 PDT
The commit-queue encountered the following flaky tests while processing attachment 88509 [details]:

http/tests/websocket/tests/multiple-connections.html bug 53825 (author: abarth@webkit.org)
The commit-queue is continuing to process your patch.
Comment 38 WebKit Commit Bot 2011-04-07 10:41:33 PDT
Comment on attachment 88509 [details]
patch v5, keep a member with URL

Clearing flags on attachment: 88509

Committed r83184: <http://trac.webkit.org/changeset/83184>
Comment 39 WebKit Commit Bot 2011-04-07 10:41:38 PDT
All reviewed patches have been landed.  Closing bug.
Comment 40 Matt Horan 2012-11-25 08:39:20 PST
This change breaks QWebFrame::url() for iframes.  The urlChanged signal is never emitted, and QWebFrame::url() returns an invalid URL.  This looks to be the source of the issues reported in bug 32723.
Comment 41 Matt Horan 2012-11-25 08:40:05 PST
Correction, bug 82995.