Bug 147227 – Crash in WebPlatformStrategies::createPingHandle - Deref a null NetworkingContext

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Brady Eidson]

Crash in WebPlatformStrategies::createPingHandle - Deref a null NetworkingContext

0   com.apple.WebKit              	0x00007fff9152d6e8 WebKit::WebFrameNetworkingContext::webFrameLoaderClient() const + 6
1   com.apple.WebKit              	0x00007fff9159b224 WebKit::WebPlatformStrategies::createPingHandle(WebCore::NetworkingContext*, WebCore::ResourceRequest&, bool) + 64
2   com.apple.WebCore             	0x00007fff97c7a696 WebCore::PingLoader::startPingLoad(WebCore::Frame&, WebCore::ResourceRequest&) + 566
3   com.apple.WebCore             	0x00007fff97c7b0f6 WebCore::PingLoader::sendViolationReport(WebCore::Frame&, WebCore::URL const&, WTF::PassRefPtr<WebCore::FormData>) + 918
4   com.apple.WebCore             	0x00007fff974f7fe2 WebCore::ContentSecurityPolicy::reportViolation(WTF::String const&, WTF::String const&, WTF::String const&, WebCore::URL const&, WTF::Vector<WebCore::URL, 0ul, WTF::CrashOnOverflow, 16ul> const&, WTF::String const&, WTF::String const&, WTF::OrdinalNumber const&, JSC::ExecState*) const + 3554
5   com.apple.WebCore             	0x00007fff974f71c8 WebCore::CSPDirectiveList::reportViolation(WTF::String const&, WTF::String const&, WTF::String const&, WebCore::URL const&, WTF::String const&, WTF::OrdinalNumber const&, JSC::ExecState*) const + 280
...
30  com.apple.WebCore             	0x00007fff9713f482 WebCore::FrameLoader::init() + 770
31  com.apple.WebKit              	0x00007fff91392fe9 WebKit::WebFrame::createSubframe(WebKit::WebPage*, WTF::String const&, WebCore::HTMLFrameOwnerElement*) + 381
32  com.apple.WebKit              	0x00007fff9152a639 WebKit::WebFrameLoaderClient::createFrame(WebCore::URL const&, WTF::String const&, WebCore::HTMLFrameOwnerElement*, WTF::String const&, bool, int, int) + 63
33  com.apple.WebCore             	0x00007fff97ece1db WebCore::SubframeLoader::loadSubframe(WebCore::HTMLFrameOwnerElement&, WebCore::URL const&, WTF::String const&, WTF::String const&) + 283

UserStyleSheet, with a font-src rule, injected into the initial empty document for a new frame, but with CSP blocking the font load, and triggering a violation report.

Wow!

Easy to repro with a layout test, though.

Radar - <rdar://problem/21949735>

[Brady Eidson]

In WK2, it would actually be possible to bypass the need for the networking context and start the load in the Networking Process.

But it's a bit unclear what we should be doing in the initial-empty-document case.  Previously, this precise violation report would not start a ping because of the null networking context, so for now let's restore that behavior.

[Brady Eidson]

Created attachment 257354 [details]
Patch v1

[Alexey Proskuryakov]

Comment on attachment 257354 [details]
Patch v1

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

Hrmpf. Nice.

> LayoutTests/http/tests/security/contentSecurityPolicy/user-style-sheet-font-crasher-expected.txt:3
> +CONSOLE MESSAGE: Refused to load the font 'http://127.0.0.1:8000/security/contentSecurityPolicy/example_font.woff' because it violates the following Content Security Policy directive: "font-src http://webkit.org".
> +
> +CONSOLE MESSAGE: Refused to load the font 'http://127.0.0.1:8000/security/contentSecurityPolicy/example_font.woff' because it violates the following Content Security Policy directive: "font-src http://webkit.org".

Why is this logged twice, do we have a bug?

> LayoutTests/http/tests/security/contentSecurityPolicy/user-style-sheet-font-crasher.html:8
> +    testRunner.addUserStyleSheet("@font-face { font-family: ExampleFont; src: url(example_font.woff); }", true);

I wonder if this can also be reproduced with something like

<iframe src="http://www.apple.com"></iframe>
<script>
frames[0].document.write(theStylesheet);
</script>

> Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.cpp:255
> +    // We shouldn't be sending ping loads during that process anyways.

What do other browsers do? I wonder if "shouldn't" may be too assertive.

It feels like we shouldn't apply user stylesheets in initial documents, what do you think?

[Brady Eidson]

(In reply to comment #3)
> Comment on attachment 257354 [details]
> Patch v1
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=257354&action=review
> 
> Hrmpf. Nice.
> 
> > LayoutTests/http/tests/security/contentSecurityPolicy/user-style-sheet-font-crasher-expected.txt:3
> > +CONSOLE MESSAGE: Refused to load the font 'http://127.0.0.1:8000/security/contentSecurityPolicy/example_font.woff' because it violates the following Content Security Policy directive: "font-src http://webkit.org".
> > +
> > +CONSOLE MESSAGE: Refused to load the font 'http://127.0.0.1:8000/security/contentSecurityPolicy/example_font.woff' because it violates the following Content Security Policy directive: "font-src http://webkit.org".
> 
> Why is this logged twice, do we have a bug?

No - the style sheet is injected into both frames, so the CSP fires twice.
> 
> > LayoutTests/http/tests/security/contentSecurityPolicy/user-style-sheet-font-crasher.html:8
> > +    testRunner.addUserStyleSheet("@font-face { font-family: ExampleFont; src: url(example_font.woff); }", true);
> 
> I wonder if this can also be reproduced with something like
> 
> <iframe src="http://www.apple.com"></iframe>
> <script>
> frames[0].document.write(theStylesheet);
> </script>
> 

That wouldn't do it, because initial document creation is synchronous, and completed by the time you get to document.write.

By the time you get there, you'd have the networking context and wouldn't repro the crash.

> > Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.cpp:255
> > +    // We shouldn't be sending ping loads during that process anyways.
> 
> What do other browsers do? I wonder if "shouldn't" may be too assertive.
> 
> It feels like we shouldn't apply user stylesheets in initial documents, what
> do you think?

Perhaps it is too assertive. I'll relax it before landing.

I postulated at least one other solution in the radar but it seems much higher risk.

I'm not sure whether or not user sheets should apply to the initial empty document - I'd want Simon, Antti, Andreas, etc to comment.

As is, this patch restores precisely the behavior we had before https://trac.webkit.org/changeset/186530 - I'm pretty confident in that interim step while we explore what we should *really* be doing here.

[Brady Eidson]

https://trac.webkit.org/changeset/187248

[Alexey Proskuryakov]

This test fails on Windows every time (maybe unsupported testRunner functionality?)

[Brady Eidson]

(In reply to comment #6)
> This test fails on Windows every time (maybe unsupported testRunner
> functionality?)

Probably. Will take a look.

[Brady Eidson]

Hmmmm, the message comes from:
- (void)webView:(WebView *)sender addMessageToConsole:(NSDictionary *)dictionary withSource:(NSString *)source
...in Mac DRT, and:
"willAddMessageToConsole"
...in WK2 WKTR.

There is a "CONSOLE MESSAGE" string in DRT Win, in:
HRESULT UIDelegate::webViewAddMessageToConsole(IWebView* /*sender*/, BSTR message, int lineNumber, BSTR url, BOOL isError)


I guess that's not getting called (or its bailing out early?)

[Alexey Proskuryakov]

I suspected that addUserStyleSheet wasn't implemented, but looks like it is.

[Brady Eidson]

(In reply to comment #9)
> I suspected that addUserStyleSheet wasn't implemented, but looks like it is.

Is CSP not working in WebKitWin? That would explain it, too.