Bug 58847

Summary: [Qt] REGRESSION(r84099): It made two Qt API tests fail
Product: WebKit Reporter: Csaba Osztrogon√°c <ossy>
Component: Tools / TestsAssignee: Rafael Brandao <rafael.lobo>
Status: RESOLVED FIXED    
Severity: Critical CC: abarth, ademar, benjamin, cmarcelo, commit-queue, kling, laszlo.gombos, menard, ossy, rafael.lobo, sam
Priority: P1 Keywords: Qt, QtTriaged
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 38654    
Attachments:
Description Flags
Changed baseURL to allow them to create a local storage when needed.
benjamin: review-
The failing test now works and tests what it was supposed to do, but also added a new one to make sure the opposite scenario wouldn't work.
none
modified new test so it could also check for positive scenario (also changed its name) none

Description Csaba Osztrogon√°c 2011-04-18 15:59:04 PDT
FAIL!  : tst_QWebPage::testOptionalJSObjects() Compared values are not the same
   Actual (testFlag(webPage2, QWebSettings::LocalStorageEnabled, "localStorage", true)): 0
   Expected (true): 1
   Loc: [/home/webkitbuildbot/slaves/release32bit/buildslave/qt-linux-release/build/Source/WebKit/qt/tests/qwebpage/tst_qwebpage.cpp(2275)]

FAIL!  : tst_QWebPage::errorPageExtensionInFrameset() Received a fatal error.
   Loc: [Unknown file(0)]
Comment 1 Rafael Brandao 2011-04-28 16:45:17 PDT
I may have found a piece of contradiction on SecurityOrigin's constructor. You can read the following:

    // These protocols do not create security origins; the owner frame provides the origin
    if (m_protocol == "about" || m_protocol == "javascript")
        m_protocol = "";

    (...)

    if (m_protocol.isEmpty())
        m_isUnique = true;


If it is marked as unique, then it will be created. This check for empty protocol was added on r84099.

But in this test, you get an empty url as a parameter on constructor, and then it sets its protocol as empty, so it'll mark its security origin as unique, and this makes impossible to make use of local storage.

I'll try to figure out why it's coming there already empty. If anyone knows more about this, I would appreciate some help.
Comment 2 Adam Barth 2011-04-28 17:35:42 PDT
> If it is marked as unique, then it will be created.

This is the constructor.  We can't avoid creating an object.

> But in this test, you get an empty url as a parameter on constructor, and then it sets its protocol as empty, so it'll mark its security origin as unique, and this makes impossible to make use of local storage.

Correct.  URLs with empty schemes can't use local storage.
Comment 3 Rafael Brandao 2011-05-03 13:48:56 PDT
I've just realized that if instead of setting HTML without a baseURL you set this base URL to any valid url, like "http://localhost" or, let's say, "http://google.com", the tests will pass. The difference is this:

webPage.currentFrame()->setHtml(QString("<html><body>test</body></html>"), QUrl()); // This cannot access local storage.

webPage.currentFrame()->setHtml(QString("<html><body>test</body></html>"), QUrl("http://localhost")); // And this can access local storage.

As user of QWebPage, I would think that not setting a baseURL would be the same as using "about:blank" as baseURL. Indeed it is working like this, but it looks like it's the opposite. If you pass "about:blank" it will change it to a blank baseURL. As it is blank url, it won't recognize any protocol on it, not allowing it to make any use of local storage. This check was added to address a previous bug, so I'm puzzled now. In this case, when the user wants to make use of local storage with his own substitute HTML, should we change the baseURL to allow him to do so? If so, what would be the best to put, localhost? Or it's best for him to disable this feature and put a note in the documentation that he won't be able to do it (by the way I don't like this idea)?

I can't help much but to ask, so I'll appreciate any feedback. :)
Comment 4 Rafael Brandao 2011-05-10 11:32:30 PDT
Hey Laszlo, what do you think about this change to allow your test to work? I really don't know what it should be, as about:blank origins are now globally unique and cannot use local storage. Perhaps adding a QWebSetting to force it to create the security origin would be a great solution. Do you have any suggestion?

I've also noticed that in this test the local storage path is not set, so it creates a local storage that is not persistent, which is a bit weird. But in this test it may not be important.

Any feedback will be greatly appreciated. :)
Comment 5 Rafael Brandao 2011-05-10 11:37:28 PDT
I forgot to say that this strange behavior (not persistent local storage) was already predicted in WebKit, but never confirmed: https://bugs.webkit.org/show_bug.cgi?id=25894
Comment 6 Caio Marcelo de Oliveira Filho 2011-05-10 11:52:00 PDT
(In reply to comment #4)
> Do you have any suggestion?

Rafael, I think that you should change this test to get a proper baseUrl, since the point of the test is not about whether we have a baseUrl or not -- but about how QWebSettings affect JS objects available. And note that test is still "broken" (QEXPECT_FAIL just tell us we know it's broken), see bug 29867.

Then create another test (and a new bug) that will explicitly check the behavior we expect for setHtml() regarding local storage (with empty baseUrl, and also specified baseUrl).
Comment 7 Rafael Brandao 2011-05-10 15:22:48 PDT
Created attachment 93026 [details]
Changed baseURL to allow them to create a local storage when needed.

This patch fixes this test so it can test what it was designed to test. But I've reported the bug that was found (https://bugs.webkit.org/show_bug.cgi?id=60589), so we will hopefully find a great solution to it.
Comment 8 Benjamin Poulain 2011-05-10 16:07:28 PDT
Comment on attachment 93026 [details]
Changed baseURL to allow them to create a local storage when needed.

I think having a valid security origin in order to get local storage is reasonable.

But, I r- because I think you should have test for both behavior. Testing that "no security origin"->"local storage disabled" would be good to avoid future regressions.
Comment 9 Rafael Brandao 2011-05-11 14:14:41 PDT
Ok, in this case I'll modify testOptionalJSObjects to test it in both scenarios. Thanks a lot for your input. :)
Comment 10 Rafael Brandao 2011-05-11 16:52:18 PDT
Created attachment 93214 [details]
The failing test now works and tests what it was supposed to do, but also added a new one to make sure the opposite scenario wouldn't work.

I've originally thought to modify testOptionalJSObjects, but I've found myself with a big test. So I decided to only change it to work and then added the new test which does some tests to assure that local storage can't be used.
Comment 11 Alexis Menard (darktears) 2011-05-12 12:15:43 PDT
Comment on attachment 93214 [details]
The failing test now works and tests what it was supposed to do, but also added a new one to make sure the opposite scenario wouldn't work.

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

> Source/WebKit/qt/tests/qwebpage/tst_qwebpage.cpp:2301
> +static inline bool checkLocalStorageVisibility(QWebPage& webPage, bool localStorageEnabled)

Why inline? It's tests, performance does not matter. :D
Comment 12 Alexis Menard (darktears) 2011-05-12 12:16:19 PDT
(In reply to comment #11)
> (From update of attachment 93214 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=93214&action=review
> 
> > Source/WebKit/qt/tests/qwebpage/tst_qwebpage.cpp:2301
> > +static inline bool checkLocalStorageVisibility(QWebPage& webPage, bool localStorageEnabled)
> 
> Why inline? It's tests, performance does not matter. :D

Or does not count as much as other places. But it's no big deal.
Comment 13 Rafael Brandao 2011-05-12 12:18:09 PDT
I agree. I was just following the pattern I've saw there. :P
But look, if you perform tests quickly, wouldn't that be a good thing? :)
Comment 14 Antonio Gomes 2011-05-25 06:30:03 PDT
Comment on attachment 93214 [details]
The failing test now works and tests what it was supposed to do, but also added a new one to make sure the opposite scenario wouldn't work.

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

> Source/WebKit/qt/tests/qwebpage/tst_qwebpage.cpp:2287
> +    webPage1.currentFrame()->setHtml(QString("<html><body>test</body></html>"), QUrl("http://www.example.com"));
> +    webPage2.currentFrame()->setHtml(QString("<html><body>test</body></html>"), QUrl("http://www.example.com"));
>  

I would not use http:// here ... a local unexistent path would not be enough?
Comment 15 Rafael Brandao 2011-05-25 06:59:50 PDT
(In reply to comment #14)
> (From update of attachment 93214 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=93214&action=review
> 
> > Source/WebKit/qt/tests/qwebpage/tst_qwebpage.cpp:2287
> > +    webPage1.currentFrame()->setHtml(QString("<html><body>test</body></html>"), QUrl("http://www.example.com"));
> > +    webPage2.currentFrame()->setHtml(QString("<html><body>test</body></html>"), QUrl("http://www.example.com"));
> >  
> 
> I would not use http:// here ... a local unexistent path would not be enough?

It would work as well, but I've used that example url because I've noticed people using it very often on those tests. But it won't actually load anything from that page because I've specified a substitute data on it, if that's the concern.
Comment 16 Andreas Kling 2011-06-02 12:55:28 PDT
Comment on attachment 93214 [details]
The failing test now works and tests what it was supposed to do, but also added a new one to make sure the opposite scenario wouldn't work.

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

Like mentioned on IRC, can we get a test where checkLocalStorageVisiblity() returns true, as well?

> Source/WebKit/qt/ChangeLog:10
> +        Specified a valid url to allow the failing test to work and
> +        get its security origin. Also added a new test to check the
> +        opposite situation, when the origin is globally unique, so
> +        local storage can't be used.
> +        
> +        https://bugs.webkit.org/show_bug.cgi?id=58847

The format for ChangeLog is typically:
$BugName
$BugURL

$MoreInformation

> Source/WebKit/qt/tests/qwebpage/tst_qwebpage.cpp:2309
> +    // Local storage's accessibility depends on its security origin, which depends on base url.

accessibility -> visibility

> Source/WebKit/qt/tests/qwebpage/tst_qwebpage.cpp:2319
> +    QCOMPARE(checkLocalStorageVisibility(webPage, true),  false);

Two spaces.

> Source/WebKit/qt/tests/qwebpage/tst_qwebpage.cpp:2324
> +    QCOMPARE(checkLocalStorageVisibility(webPage, true),  false);

Ditto.

> Source/WebKit/qt/tests/qwebpage/tst_qwebpage.cpp:2329
> +    QCOMPARE(checkLocalStorageVisibility(webPage, true),  false);

Ditto.

> Source/WebKit/qt/tests/qwebpage/tst_qwebpage.cpp:2334
> +    QCOMPARE(checkLocalStorageVisibility(webPage, true),  false);

Ditto.
Comment 17 Rafael Brandao 2011-06-03 11:51:54 PDT
Created attachment 95939 [details]
modified new test so it could also check for positive scenario (also changed its name)

I've added more negative tests in the meantime, and the new ones that would make use of local storage. I was wondering if I should keep this in two separated tests, but for now it seemed right to me to keep them all together as it would be better for understanding the relationship between base url and local storage.

Any feedbacks are welcome. :)
Comment 18 Andreas Kling 2011-06-03 13:03:54 PDT
Comment on attachment 95939 [details]
modified new test so it could also check for positive scenario (also changed its name)

Glorious! r=me
Comment 19 WebKit Commit Bot 2011-06-03 14:53:16 PDT
Comment on attachment 95939 [details]
modified new test so it could also check for positive scenario (also changed its name)

Clearing flags on attachment: 95939

Committed r88065: <http://trac.webkit.org/changeset/88065>
Comment 20 WebKit Commit Bot 2011-06-03 14:53:24 PDT
All reviewed patches have been landed.  Closing bug.
Comment 21 Ademar Reis 2011-06-06 08:20:59 PDT
Revision r88065 cherry-picked into qtwebkit-2.2 with commit 98ac1d2 <http://gitorious.org/webkit/qtwebkit/commit/98ac1d2>