Bug 32723 – [Qt] QWebFrame::setUrl works only from second time if url fragment is present

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Qt

[Rion]

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

[Rion]

load method works fine with any url

[Tor Arne Vestbø]

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

[Kent Hansen]

Reproduced on Linux against r55658.

[Kent Hansen]

Created attachment 50487 [details]
Testcase

Note: this breaks kchmviewer application http://www.kchmviewer.net/

[Jenya Brodskaia]

Cannot reproduce on Linux for r71415

Still reproducible in qt-4.7.1. Jenya was that revision supposed to be in 4.7.1?

[Jenya Brodskaia]

No, that was on webkit trunk.

[Benjamin Poulain]

*** Bug 55370 has been marked as a duplicate of this bug. ***

[Caio Marcelo de Oliveira Filho]

Reproduced with Linux 64 bits, webkit trunk r80142 and Qt from 4.7 branch commit b31b4c46e70401745572b808c916088afb839614.

Investigating the problem.

[Caio Marcelo de Oliveira Filho]

Created attachment 84489 [details]
patch

[Alexis Menard (darktears)]

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?

[Caio Marcelo de Oliveira Filho]

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

[Caio Marcelo de Oliveira Filho]

Comment on attachment 84489 [details]
patch

Investigating a possible regression caused by this change in one of the tests.

[Antonio Gomes]

Comment on attachment 84489 [details]
patch

Caio, I think it needs tests.

r- due to the lack of tests.

[Aparna Nandyal]

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.

[Caio Marcelo de Oliveira Filho]

Created attachment 84601 [details]
autotest for qt api

This is not the fix, just the autotest.

[WebKit Commit Bot]

Comment on attachment 84601 [details]
autotest for qt api

Clearing flags on attachment: 84601

Committed r80314: <http://trac.webkit.org/changeset/80314>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Caio Marcelo de Oliveira Filho]

Reopening since the bug is still there.

[Caio Marcelo de Oliveira Filho]

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.

[Caio Marcelo de Oliveira Filho]

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.

[Caio Marcelo de Oliveira Filho]

Created attachment 85471 [details]
patch v2, all qt api tests pass now

[Benjamin Poulain]

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.

[Caio Marcelo de Oliveira Filho]

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.

[Benjamin Poulain]

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.

[Benjamin Poulain]

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

[Caio Marcelo de Oliveira Filho]

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.

[Caio Marcelo de Oliveira Filho]

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?

[Benjamin Poulain]

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 :)

[Caio Marcelo de Oliveira Filho]

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).

[Kenneth Rohde Christiansen]

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?

[Caio Marcelo de Oliveira Filho]

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

[Caio Marcelo de Oliveira Filho]

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.

[Caio Marcelo de Oliveira Filho]

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.

[Benjamin Poulain]

Comment on attachment 88509 [details]
patch v5, keep a member with URL

Looks good

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 88509 [details]:

http/tests/websocket/tests/multiple-connections.html bug 53825 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

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>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Matt Horan]

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.

[Matt Horan]

Correction, bug 82995.