58847 – [Qt] REGRESSION(r84099): It made two Qt API tests fail

Csaba Osztrogonác

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

Rafael Brandao

Comment 1 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.

Adam Barth

Comment 2 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.

Rafael Brandao

Comment 3 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. :)

Rafael Brandao

Comment 4 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. :)

Rafael Brandao

Comment 5 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

Caio Marcelo de Oliveira Filho

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

Rafael Brandao

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

Benjamin Poulain

Comment 8 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.

Rafael Brandao

Comment 9 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. :)

Rafael Brandao

Comment 10 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.

Alexis Menard (darktears)

Comment 11 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

Alexis Menard (darktears)

Comment 12 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.

Rafael Brandao

Comment 13 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? :)

Antonio Gomes

Comment 14 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?

Rafael Brandao

Comment 15 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.

Andreas Kling

Comment 16 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.

Rafael Brandao

Comment 17 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. :)

Andreas Kling

Comment 18 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

WebKit Commit Bot

Comment 19 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>

WebKit Commit Bot

Comment 20 2011-06-03 14:53:24 PDT

All reviewed patches have been landed. Closing bug.

Ademar Reis

Comment 21 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>

Attachments
Changed baseURL to allow them to create a local storage when needed. (1.68 KB, patch) 2011-05-10 15:22 PDT, Rafael Brandao	benjamin: review-	Details Formatted Diff Diff
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. (4.01 KB, patch) 2011-05-11 16:52 PDT, Rafael Brandao	no flags	Details Formatted Diff Diff
modified new test so it could also check for positive scenario (also changed its name) (6.37 KB, patch) 2011-06-03 11:51 PDT, Rafael Brandao	no flags	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.