| Summary: | [Qt] REGRESSION(r84099): It made two Qt API tests fail | ||
|---|---|---|---|
| Product: | WebKit | Reporter: | Csaba Osztrogonác <ossy> |
| Component: | Tools / Tests | Assignee: | 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
Csaba Osztrogonác
2011-04-18 15:59:04 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.
> 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. 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. :) 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. :) 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 (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). 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 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.
Ok, in this case I'll modify testOptionalJSObjects to test it in both scenarios. Thanks a lot for your input. :) 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 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 (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. 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 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? (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 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. 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 on attachment 95939 [details]
modified new test so it could also check for positive scenario (also changed its name)
Glorious! r=me
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> All reviewed patches have been landed. Closing bug. Revision r88065 cherry-picked into qtwebkit-2.2 with commit 98ac1d2 <http://gitorious.org/webkit/qtwebkit/commit/98ac1d2> |