Bug 167050

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 Flags
Layout test that only triggers bug through manual clicks, not eventSender
none
Patch
none
Layout Test
none
Patch
none
Patch
none
Patch
none
Patch for landing none

Description Michael Catanzaro 2017-01-14 10:20:08 PST
This assertion is reproducible on https://trac.webkit.org/browser:

(1) Click on one of the four expander arrows, e.g. the one immediately to the left of "trunk" in order to expand the directory layout. Note that, while the contents of the page change, the page URL remains https://trac.webkit.org/browser.
(2) Click on any link, e.g. the WebKit logo in the top left of the page. Wait for the page to load.
(3) Click Back. Note the page URL is now different, it has gained a fragment: https://trac.webkit.org/browser#trunk
(4) Click on any link, e.g. the WebKit logo in the top-left again.

Now in WebCore::SecurityPolicy::generateReferrerHeader, we have:

 * referrer is "https://trac.webkit.org/browser#trunk"
 * URL(URL(), referrer).strippedForUseAsReferrer() is "https://trac.webkit.org/browser"

URL::strippedForUseAsReferrer intentionally removes the fragment, as required by RFC 2616. So referrer is somehow incorrect here. It triggers this assertion:

ASSERTION FAILED: referrer == URL(URL(), referrer).strippedForUseAsReferrer()
../../Source/WebCore/page/SecurityPolicy.cpp(73) : static WTF::String WebCore::SecurityPolicy::generateReferrerHeader(WebCore::ReferrerPolicy, const WebCore::URL&, const WTF::String&)
1   0x7f0f496b92d2 /home/mcatanzaro/Projects/GNOME/install/lib/libjavascriptcoregtk-4.0.so.18(WTFCrash+0x1e) [0x7f0f496b92d2]
2   0x7f0f52b0c877 /home/mcatanzaro/Projects/GNOME/install/lib/libwebkit2gtk-4.0.so.37(_ZN7WebCore14SecurityPolicy22generateReferrerHeaderENS_14ReferrerPolicyERKNS_3URLERKN3WTF6StringE+0x1dd) [0x7f0f52b0c877]
3   0x7f0f5294ad37 /home/mcatanzaro/Projects/GNOME/install/lib/libwebkit2gtk-4.0.so.37(_ZN7WebCore11FrameLoader16loadFrameRequestERKNS_16FrameLoadRequestEPNS_5EventEPNS_9FormStateE+0x265) [0x7f0f5294ad37]
4   0x7f0f52947553 /home/mcatanzaro/Projects/GNOME/install/lib/libwebkit2gtk-4.0.so.37(_ZN7WebCore11FrameLoader11urlSelectedERKNS_16FrameLoadRequestEPNS_5EventE+0x23b) [0x7f0f52947553]
5   0x7f0f529472c1 /home/mcatanzaro/Projects/GNOME/install/lib/libwebkit2gtk-4.0.so.37(_ZN7WebCore11FrameLoader11urlSelectedERKNS_3URLERKN3WTF6StringEPNS_5EventENS_11LockHistoryENS_19LockBackForwardListENS_18ShouldSendReferrerENS_28ShouldOpenExternalURLsPolicyESt8optionalINS_20NewFrameOpenerPolicyEERKNS4_12AtomicStringE+0x10d) [0x7f0f529472c1]
6   0x7f0f52698211 /home/mcatanzaro/Projects/GNOME/install/lib/libwebkit2gtk-4.0.so.37(_ZN7WebCore17HTMLAnchorElement11handleClickERNS_5EventE+0x387) [0x7f0f52698211]
7   0x7f0f52696f8c /home/mcatanzaro/Projects/GNOME/install/lib/libwebkit2gtk-4.0.so.37(_ZN7WebCore17HTMLAnchorElement19defaultEventHandlerERNS_5EventE+0xe8) [0x7f0f52696f8c]
8   0x7f0f52492894 /home/mcatanzaro/Projects/GNOME/install/lib/libwebkit2gtk-4.0.so.37(+0x5e28894) [0x7f0f52492894]
9   0x7f0f52492f78 /home/mcatanzaro/Projects/GNOME/install/lib/libwebkit2gtk-4.0.so.37(_ZN7WebCore15EventDispatcher13dispatchEventERNS_4NodeERNS_5EventE+0x3cf) [0x7f0f52492f78]
10  0x7f0f524e0bad /home/mcatanzaro/Projects/GNOME/install/lib/libwebkit2gtk-4.0.so.37(_ZN7WebCore4Node13dispatchEventERNS_5EventE+0x53) [0x7f0f524e0bad]
11  0x7f0f5247311a /home/mcatanzaro/Projects/GNOME/install/lib/libwebkit2gtk-4.0.so.37(_ZN7WebCore7Element18dispatchMouseEventERKNS_18PlatformMouseEventERKN3WTF12AtomicStringEiPS0_+0x19c) [0x7f0f5247311a]
12  0x7f0f52a877b5 /home/mcatanzaro/Projects/GNOME/install/lib/libwebkit2gtk-4.0.so.37(_ZN7WebCore12EventHandler18dispatchMouseEventERKN3WTF12AtomicStringEPNS_4NodeEbiRKNS_18PlatformMouseEventEb+0xfd) [0x7f0f52a877b5]
13  0x7f0f52a85409 /home/mcatanzaro/Projects/GNOME/install/lib/libwebkit2gtk-4.0.so.37(_ZN7WebCore12EventHandler23handleMouseReleaseEventERKNS_18PlatformMouseEventE+0x60d) [0x7f0f52a85409]
14  0x7f0f530f85dc /home/mcatanzaro/Projects/GNOME/install/lib/libwebkit2gtk-4.0.so.37(_ZN7WebCore15UserInputBridge23handleMouseReleaseEventERKNS_18PlatformMouseEventENS_11InputSourceE+0x3c) [0x7f0f530f85dc]
15  0x7f0f519ed152 /home/mcatanzaro/Projects/GNOME/install/lib/libwebkit2gtk-4.0.so.37(+0x5383152) [0x7f0f519ed152]
16  0x7f0f519ed436 /home/mcatanzaro/Projects/GNOME/install/lib/libwebkit2gtk-4.0.so.37(_ZN6WebKit7WebPage10mouseEventERKNS_13WebMouseEventE+0x1f4) [0x7f0f519ed436]
17  0x7f0f51d79df1 /home/mcatanzaro/Projects/GNOME/install/lib/libwebkit2gtk-4.0.so.37(_ZN3IPC22callMemberFunctionImplIN6WebKit7WebPageEMS2_FvRKNS1_13WebMouseEventEESt5tupleIJS3_EEJLm0EEEEvPT_T0_OT1_St16integer_sequenceImJXspT2_EEE+0x80) [0x7f0f51d79df1]
18  0x7f0f51d7838a /home/mcatanzaro/Projects/GNOME/install/lib/libwebkit2gtk-4.0.so.37(_ZN3IPC18callMemberFunctionIN6WebKit7WebPageEMS2_FvRKNS1_13WebMouseEventEESt5tupleIJS3_EESt16integer_sequenceImJLm0EEEEEvOT1_PT_T0_+0x41) [0x7f0f51d7838a]
19  0x7f0f51d716ee /home/mcatanzaro/Projects/GNOME/install/lib/libwebkit2gtk-4.0.so.37(_ZN3IPC13handleMessageIN8Messages7WebPage10MouseEventEN6WebKit7WebPageEMS5_FvRKNS4_13WebMouseEventEEEEvRNS_7DecoderEPT0_T1_+0x9b) [0x7f0f51d716ee]
20  0x7f0f51d6c68d /home/mcatanzaro/Projects/GNOME/install/lib/libwebkit2gtk-4.0.so.37(_ZN6WebKit7WebPage24didReceiveWebPageMessageERN3IPC10ConnectionERNS1_7DecoderE+0x597) [0x7f0f51d6c68d]
21  0x7f0f519f37ca /home/mcatanzaro/Projects/GNOME/install/lib/libwebkit2gtk-4.0.so.37(_ZN6WebKit7WebPage17didReceiveMessageERN3IPC10ConnectionERNS1_7DecoderE+0x240) [0x7f0f519f37ca]
22  0x7f0f515e55b5 /home/mcatanzaro/Projects/GNOME/install/lib/libwebkit2gtk-4.0.so.37(_ZN3IPC18MessageReceiverMap15dispatchMessageERNS_10ConnectionERNS_7DecoderE+0x125) [0x7f0f515e55b5]
23  0x7f0f51872f8a /home/mcatanzaro/Projects/GNOME/install/lib/libwebkit2gtk-4.0.so.37(_ZN6WebKit10WebProcess17didReceiveMessageERN3IPC10ConnectionERNS1_7DecoderE+0x4c) [0x7f0f51872f8a]
24  0x7f0f515cae6a /home/mcatanzaro/Projects/GNOME/install/lib/libwebkit2gtk-4.0.so.37(_ZN3IPC10Connection15dispatchMessageERNS_7DecoderE+0x3a) [0x7f0f515cae6a]
25  0x7f0f515cafd4 /home/mcatanzaro/Projects/GNOME/install/lib/libwebkit2gtk-4.0.so.37(_ZN3IPC10Connection15dispatchMessageESt10unique_ptrINS_7DecoderESt14default_deleteIS2_EE+0x166) [0x7f0f515cafd4]
26  0x7f0f515cb1c6 /home/mcatanzaro/Projects/GNOME/install/lib/libwebkit2gtk-4.0.so.37(_ZN3IPC10Connection18dispatchOneMessageEv+0xc8) [0x7f0f515cb1c6]
27  0x7f0f515cad0a /home/mcatanzaro/Projects/GNOME/install/lib/libwebkit2gtk-4.0.so.37(+0x4f60d0a) [0x7f0f515cad0a]
28  0x7f0f515d10aa /home/mcatanzaro/Projects/GNOME/install/lib/libwebkit2gtk-4.0.so.37(+0x4f670aa) [0x7f0f515d10aa]
29  0x7f0f5159456d /home/mcatanzaro/Projects/GNOME/install/lib/libwebkit2gtk-4.0.so.37(_ZNK3WTF8FunctionIFvvEEclEv+0x37) [0x7f0f5159456d]
30  0x7f0f496d8a68 /home/mcatanzaro/Projects/GNOME/install/lib/libjavascriptcoregtk-4.0.so.18(_ZN3WTF7RunLoop11performWorkEv+0xce) [0x7f0f496d8a68]
31  0x7f0f4971f8e8 /home/mcatanzaro/Projects/GNOME/install/lib/libjavascriptcoregtk-4.0.so.18(+0x25008e8) [0x7f0f4971f8e8]
Comment 1 Alexey Proskuryakov 2017-01-17 16:45:09 PST
Reproduces on Mac.

rdar://problem/27972404
Comment 2 Alex Christensen 2017-01-18 11:39:46 PST
Looks like we need to strip the fragment somewhere
Comment 3 John Wilander 2017-02-01 11:27:34 PST
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.
Comment 4 John Wilander 2017-02-01 11:32:25 PST
Created attachment 300342 [details]
Layout test that only triggers bug through manual clicks, not eventSender
Comment 5 John Wilander 2017-02-01 12:11:49 PST
Created attachment 300344 [details]
Patch
Comment 6 Michael Catanzaro 2017-02-01 13:57:10 PST
You could add it to the ManualTests directory, although this is not great. Strange that eventSender doesn't work....
Comment 7 Alexey Proskuryakov 2017-02-01 14:32:36 PST
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.
Comment 8 John Wilander 2017-02-01 14:38:04 PST
(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"
Comment 9 Daniel Bates 2017-02-06 14:00:14 PST
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.
Comment 10 Fujii Hironori 2019-08-23 01:18:44 PDT
Created attachment 377114 [details]
Patch
Comment 11 Fujii Hironori 2019-08-23 01:29:02 PDT
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 12 youenn fablet 2019-08-23 05:52:45 PDT
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 13 Alexey Proskuryakov 2019-08-23 09:29:28 PDT
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.
Comment 14 Fujii Hironori 2019-08-25 23:29:29 PDT
Created attachment 377227 [details]
Patch
Comment 15 Fujii Hironori 2019-08-25 23:32:48 PDT
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 ?
Comment 16 Fujii Hironori 2019-08-25 23:50:40 PDT
Created attachment 377228 [details]
Patch
Comment 17 youenn fablet 2019-08-26 04:20:31 PDT
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).
Comment 18 Alexey Proskuryakov 2019-08-26 09:14:56 PDT
> 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 19 Fujii Hironori 2019-08-26 21:38:47 PDT
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.
Comment 20 Fujii Hironori 2019-08-26 22:28:39 PDT
Oh, no. finishJSTest() doesn't work as expected in test cases using testRunner.queueBackNavigation.
It seems that I need to use sync XHR.
Comment 21 Fujii Hironori 2019-08-26 22:45:05 PDT
Created attachment 377320 [details]
Patch for landing
Comment 22 Fujii Hironori 2019-08-27 18:41:14 PDT
Comment on attachment 377320 [details]
Patch for landing

Clearing flags on attachment: 377320

Committed r249188: <https://trac.webkit.org/changeset/249188>
Comment 23 Fujii Hironori 2019-08-27 18:41:18 PDT
All reviewed patches have been landed.  Closing bug.