Summary: | ASSERTION FAILED: referrer == URL(URL(), referrer).strippedForUseAsReferrer() in WebCore::SecurityPolicy::generateReferrerHeader on https://trac.webkit.org/browser | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Michael Catanzaro <mcatanzaro> | ||||||||||||||||
Component: | WebCore Misc. | Assignee: | John Wilander <wilander> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | achristensen, ap, bugs-noreply, cdumez, commit-queue, dbates, Hironori.Fujii, japhet, mcatanzaro, wilander, youennf | ||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||
Hardware: | All | ||||||||||||||||||
OS: | All | ||||||||||||||||||
URL: | https://trac.webkit.org/browser | ||||||||||||||||||
Attachments: |
|
Description
Michael Catanzaro
2017-01-14 10:20:08 PST
Reproduces on Mac. rdar://problem/27972404 Looks like we need to strip the fragment somewhere My layout test only triggers the bug through manual clicks, not when using the eventSender. I'll attach my test case here separately, then upload a patch without a test. Created attachment 300342 [details]
Layout test that only triggers bug through manual clicks, not eventSender
Created attachment 300344 [details]
Patch
You could add it to the ManualTests directory, although this is not great. Strange that eventSender doesn't work.... Comment on attachment 300344 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=300344&action=review > Source/WebCore/ChangeLog:11 > + but it doesn't trigger the bug. If you run it with manual clicks it > + does trigger the bug but not when run with eventSender. This seems quite surprising! Perhaps there is a timing difference. (In reply to comment #7) > Comment on attachment 300344 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=300344&action=review > > > Source/WebCore/ChangeLog:11 > > + but it doesn't trigger the bug. If you run it with manual clicks it > > + does trigger the bug but not when run with eventSender. > > This seems quite surprising! Perhaps there is a timing difference. IIRC I experimented quite heavily with timing. But I might be missing something. As seen in the -expected.txt file all the steps are carried out which means the clicks are happening: PASS window.frames[0].document.querySelector('h1').innerText is "Page 1" PASS window.frames[0].document.location.hash is "" PASS window.frames[0].document.referrer is "http://127.0.0.1:8000/loading/open-cached-frame-should-strip-outgoing-referrer-url-for-use-as-referrer.html" PASS window.frames[0].document.querySelector('h1').innerText is "Page 1" PASS window.frames[0].document.location.hash is "#theFragment" PASS window.frames[0].document.referrer is "http://127.0.0.1:8000/loading/open-cached-frame-should-strip-outgoing-referrer-url-for-use-as-referrer.html" PASS window.frames[0].document.querySelector('h1').innerText is "Nop Page" PASS window.frames[0].document.location.hash is "" PASS window.frames[0].document.referrer is "http://127.0.0.1:8000/loading/open-cached-frame-should-strip-outgoing-referrer-url-for-use-as-referrer.html" PASS window.frames[0].document.querySelector('h1').innerText is "Page 1" PASS window.frames[0].document.location.hash is "#theFragment" PASS window.frames[0].document.referrer is "http://127.0.0.1:8000/loading/open-cached-frame-should-strip-outgoing-referrer-url-for-use-as-referrer.html" PASS window.frames[0].document.querySelector('h1').innerText is "Nop Page" PASS window.frames[0].document.location.hash is "" PASS window.frames[0].document.referrer is "http://127.0.0.1:8000/loading/open-cached-frame-should-strip-outgoing-referrer-url-for-use-as-referrer.html" Created attachment 300756 [details]
Layout Test
Attached is a layout tests that crashes with the reported assertion failure in DumpRenderTree and WebKitTestRunner. We should look to simplify this test case and assert something about the state of document.referrer.
Created attachment 377114 [details]
Patch
1. Go to https://trac.webkit.org/wiki#AboutWebKit 2. Click "WebKit Team" 3. History back 4. Click "WebKit Team" 5. The assertion fails Comment on attachment 377114 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=377114&action=review > LayoutTests/http/tests/history/fragment-change-then-back.html:26 > + }, 2000) This test will take 2 seconds to load, can we try to simplify it? Or maybe John's initial test is now passing? Comment on attachment 377114 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=377114&action=review > LayoutTests/http/tests/history/fragment-change-then-back.html:10 > + testRunner.dumpAsText(); > + testRunner.waitUntilDone(); Please use "jsTestIsAsync = true;" instead of these. > LayoutTests/http/tests/history/fragment-change-then-back.html:25 > + if (window.testRunner) > + testRunner.notifyDone(); And finishJSTest() instead of these. > LayoutTests/http/tests/history/fragment-change-then-back.html:30 > +<script src="../resources/js-test-post.js"></script> The post script should be after body. Better yet, just use js-test.js in the prefix, then no postfix is needed. > LayoutTests/http/tests/history/fragment-change-then-back.html:33 > +<p>This test PASSED if it does not cause an assertion failure.</p> If you make this a description() call, the order of messages in the output will be more logical. Created attachment 377227 [details]
Patch
Comment on attachment 377114 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=377114&action=review >> LayoutTests/http/tests/history/fragment-change-then-back.html:26 >> + }, 2000) > > This test will take 2 seconds to load, can we try to simplify it? > Or maybe John's initial test is now passing? Removed the 2 seconds by using pageshow event. >> LayoutTests/http/tests/history/fragment-change-then-back.html:30 >> +<script src="../resources/js-test-post.js"></script> > > The post script should be after body. Better yet, just use js-test.js in the prefix, then no postfix is needed. It's in LayoutTests/resources/js-test.js. Should I copy it into http/tests/resources/js-test.js ? Created attachment 377228 [details]
Patch
Comment on attachment 377228 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=377228&action=review > LayoutTests/http/tests/navigation/page-cache-fragment-referrer.html:22 > + shouldBeEqualToString('xhr.responseText', 'http://127.0.0.1:8000/navigation/page-cache-fragment-referrer.html'); Can we compute the expected value with document.location.href. Something like shouldBeEqualToString('xhr.responseText' + window.location.hash, window.location.href); Also, I would use either async XHR or fetch. jsTestIsAsync = true; window.addEventListener('pageshow', async () => { const response = fetch('resources/referrer.php'); shouldBeEqualToString('xhr.responseText' + window.location.hash, window.location.href); finishJSTest(); }); It was not initially clear to me that we were doing the XHR before going to success.html and after doing a back navigation. We could register page show after back navigation, using some state kept through local storage. Or we could queue the load/back navigation after the first xhr/fetch test, do the second xhr/fetch test and call finishJSTest (again using local storage). > It's in LayoutTests/resources/js-test.js. Should I copy it into http/tests/resources/js-test.js ?
I think that using js-test-post is OK for now, but we should look into reducing the number of harness variations (I see that there is also some mystery js-test-post-async.js in http).
Comment on attachment 377228 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=377228&action=review >> LayoutTests/http/tests/navigation/page-cache-fragment-referrer.html:22 >> + shouldBeEqualToString('xhr.responseText', 'http://127.0.0.1:8000/navigation/page-cache-fragment-referrer.html'); > > Can we compute the expected value with document.location.href. > Something like shouldBeEqualToString('xhr.responseText' + window.location.hash, window.location.href); > > Also, I would use either async XHR or fetch. > jsTestIsAsync = true; > window.addEventListener('pageshow', async () => { > const response = fetch('resources/referrer.php'); > shouldBeEqualToString('xhr.responseText' + window.location.hash, window.location.href); > finishJSTest(); > }); > > It was not initially clear to me that we were doing the XHR before going to success.html and after doing a back navigation. > > We could register page show after back navigation, using some state kept through local storage. > Or we could queue the load/back navigation after the first xhr/fetch test, do the second xhr/fetch test and call finishJSTest (again using local storage). Good idea. Will fix. I don't need local storage because I'm able to use a simple variable. Oh, no. finishJSTest() doesn't work as expected in test cases using testRunner.queueBackNavigation. It seems that I need to use sync XHR. Created attachment 377320 [details]
Patch for landing
Comment on attachment 377320 [details] Patch for landing Clearing flags on attachment: 377320 Committed r249188: <https://trac.webkit.org/changeset/249188> All reviewed patches have been landed. Closing bug. |