Bug 146096 – Crash under WebCore::PageConsoleClient::addMessage attempting to log insecure content message in ImageDocument

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Joseph Pecoraro]

* SUMMARY
Crash under WebCore::PageConsoleClient::addMessage attempting to log insecure content message in ImageDocument

* CRASH SNIPPET

Exception Type:        EXC_BAD_ACCESS (SIGSEGV)
Exception Codes:       KERN_INVALID_ADDRESS at 0x0000000000000000
Exception Note:        EXC_CORPSE_NOTIFY

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   WebCore::PageConsoleClient::addMessage(JSC::MessageSource, JSC::MessageLevel, WTF::String const&, unsigned long, WebCore::Document*) + 83
1   WebCore::MixedContentChecker::logWarning(bool, WTF::String const&, WebCore::URL const&) const + 329
2   WebCore::MixedContentChecker::canDisplayInsecureContent(WebCore::SecurityOrigin*, WebCore::MixedContentChecker::ContentType, WebCore::URL const&) const + 192
3   WebCore::CachedResourceLoader::checkInsecureContent(WebCore::CachedResource::Type, WebCore::URL const&) const + 134
4   WebCore::CachedResourceLoader::canRequest(WebCore::CachedResource::Type, WebCore::URL const&, WebCore::ResourceLoaderOptions const&, bool) + 681
5   WebCore::CachedResourceLoader::requestResource(WebCore::CachedResource::Type, WebCore::CachedResourceRequest&) + 355
6   WebCore::CachedResourceLoader::requestFont(WebCore::CachedResourceRequest&, bool) + 67
7   WebCore::CSSFontFaceSrcValue::cachedFont(WebCore::Document*, bool) + 852
8   WebCore::CSSFontSelector::addFontFaceRule(WebCore::StyleRuleFontFace const*) + 1559
9   WebCore::RuleSet::addChildRules(WTF::Vector<WTF::RefPtr<WebCore::StyleRuleBase>, 0ul, WTF::CrashOnOverflow, 16ul> const&, WebCore::MediaQueryEvaluator const&, WebCore::StyleResolver*, bool, WebCore::AddRuleFlags) + 398
10  WebCore::RuleSet::addRulesFromSheet(WebCore::StyleSheetContents*, WebCore::MediaQueryEvaluator const&, WebCore::StyleResolver*) + 193
11  WebCore::DocumentRuleSets::initUserStyle(WebCore::DocumentStyleSheetCollection&, WebCore::MediaQueryEvaluator const&, WebCore::StyleResolver&) + 505
12  WebCore::StyleResolver::StyleResolver(WebCore::Document&, bool) + 1257
13  WebCore::Document::createStyleResolver() + 82
14  WebCore::Style::attachRenderTree(WebCore::Element&, WebCore::RenderStyle&, WebCore::RenderTreePosition&, WTF::PassRefPtr<WebCore::RenderStyle>) + 669
15  WebCore::Style::resolveTree(WebCore::Element&, WebCore::RenderStyle&, WebCore::RenderTreePosition&, WebCore::Style::Change) + 713
16  WebCore::Style::resolveTree(WebCore::Document&, WebCore::Style::Change) + 328
17  WebCore::Document::recalcStyle(WebCore::Style::Change) + 281
18  WebCore::ImageDocument::imageUpdated() + 26
19  WebCore::ImageDocument::updateDuringParsing() + 150
20  WebCore::DocumentLoader::commitData(char const*, unsigned long) + 579
21  WebKit::WebFrameLoaderClient::committedLoad(WebCore::DocumentLoader*, char const*, int) + 50
22  WebCore::DocumentLoader::commitLoad(char const*, int) + 145
23  WebCore::CachedRawResource::notifyClientsDataWasReceived(char const*, unsigned int) + 160
24  WebCore::CachedRawResource::addDataBuffer(WebCore::SharedBuffer&) + 145
25  WebCore::SubresourceLoader::didReceiveDataOrBuffer(char const*, int, WTF::PassRefPtr<WebCore::SharedBuffer>, long long, WebCore::DataPayloadType) + 210
26  WebCore::SubresourceLoader::didReceiveData(char const*, unsigned int, long long, WebCore::DataPayloadType) + 35
27  WebKit::WebResourceLoader::didReceiveWebResourceLoaderMessage(IPC::Connection&, IPC::MessageDecoder&) + 257
28  IPC::Connection::dispatchMessage(std::__1::unique_ptr<IPC::MessageDecoder, std::__1::default_delete<IPC::MessageDecoder> >) + 102
29  IPC::Connection::dispatchOneMessage() + 114

* NOTES
Nothing in MixedContentChecker looks suspicious in its call to Document::addConsoleMessage given the surrounding code. But it is espeically suspicous that the crash happened inside of this signature of addMessage, which eventually just calls through to the others, so the crash likely happened before it got to pass through.

This looks particularly suspicious:

    ScriptableDocumentParser* parser = document->scriptableDocumentParser();
    …
    if (!parser->shouldAssociateConsoleMessagesWithTextPosition())
        return;

Because apparently Document::scriptableDocumentParser can return nullptr.

    ScriptableDocumentParser* Document::scriptableDocumentParser() const
    {
        return parser() ? parser()->asScriptableDocumentParser() : nullptr;
    }

An early return should be fine.

[Joseph Pecoraro]

<rdar://problem/21423510>

[Joseph Pecoraro]

I was unable to write a test for this, but no doubt it is possible. Any pointers would be very helpful. I attempted to write a test in LayoutTests/http/tests/security/mixedContent loading an SVG document that loads an insecure CSS font. It seems I wasn't able to trigger the ImageDocument path. Likewise loading the SVG directly over https didn't reproduce the issue for me.

That said, I believe the fix is rather easy to follow from the crash log, and just makes sense. So I'm proposing the fix without a test.

[Joseph Pecoraro]

Created attachment 255080 [details]
[PATCH] Proposed Fix

[Brady Eidson]

(In reply to comment #2)
> I was unable to write a test for this, but no doubt it is possible. Any
> pointers would be very helpful. I attempted to write a test in
> LayoutTests/http/tests/security/mixedContent loading an SVG document that
> loads an insecure CSS font. It seems I wasn't able to trigger the
> ImageDocument path. Likewise loading the SVG directly over https didn't
> reproduce the issue for me.

ImageDocuments are solely for PDFs or bitmap images loaded as main resources. You'll never be able to trigger this for an SVG.

I think the key to writing a test will be installing a user stylesheet (as indicated in the backtrace) that would trigger the insecure load, and then redirecting the test to an image-as-main-resource.

[Brady Eidson]

Removing CQ+ because I believe Joe was going to explore a layout test with the hint on how to do so.

[Joseph Pecoraro]

Created attachment 255151 [details]
[PATCH] Proposed Fix - With Test

Thanks Brady! That worked perfectly. Now with a test!

[Joseph Pecoraro]

Comment on attachment 255151 [details]
[PATCH] Proposed Fix - With Test

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

> LayoutTests/http/tests/security/mixedContent/insecure-content-in-image-document.html:17
> +    testRunner.addUserStyleSheet(`
> +        @font-face {
> +        	font-family: SpaceOnly;
> +        	src: url('http://127.0.0.1:8080/security/resources/SpaceOnly.otf');
> +        }
> +        * {
> +            font-family: SpaceOnly;
> +        }
> +    `, true);

I believe all ports support ES6_TEMPLATE_LITERAL_SYNTAX, but it is behind a compile time flag... I could convert this to a regular string if necessary.

[Build Bot]

Comment on attachment 255151 [details]
[PATCH] Proposed Fix - With Test

Attachment 255151 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5638392255610880

New failing tests:
http/tests/security/mixedContent/insecure-content-in-image-document.html

[Build Bot]

Created attachment 255154 [details]
Archive of layout-test-results from ews103 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Joseph Pecoraro]

> New failing tests:
> http/tests/security/mixedContent/insecure-content-in-image-document.html

I'm starting to think the bots just don't like me... I cannot reproduce this failure.

[Joseph Pecoraro]

Created attachment 255207 [details]
[PATCH] Proposed Fix - Doubled timeout in test

Doubling the timeout from 100ms to 200ms to see if that could be related the issue seen on the bots. If it is timing, some event to know when the sub-window completed loading would be useful.

[Build Bot]

Comment on attachment 255207 [details]
[PATCH] Proposed Fix - Doubled timeout in test

Attachment 255207 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6418407721271296

New failing tests:
http/tests/security/mixedContent/insecure-content-in-image-document.html

[Build Bot]

Created attachment 255210 [details]
Archive of layout-test-results from ews103 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Joseph Pecoraro]

Created attachment 255223 [details]
[PATCH] Proposed Fix - Alternate Test Approach

Trying a different test approach, no setTimeout.

[Build Bot]

Comment on attachment 255223 [details]
[PATCH] Proposed Fix - Alternate Test Approach

Attachment 255223 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5704227661807616

New failing tests:
http/tests/security/mixedContent/insecure-content-in-image-document.html

[Build Bot]

Created attachment 255226 [details]
Archive of layout-test-results from ews101 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Joseph Pecoraro]

(In reply to comment #15)
> Comment on attachment 255223 [details]
> [PATCH] Proposed Fix - Alternate Test Approach
> 
> Attachment 255223 [details] did not pass mac-ews (mac):
> Output: http://webkit-queues.appspot.com/results/5704227661807616
> 
> New failing tests:
> http/tests/security/mixedContent/insecure-content-in-image-document.html

Ahh interesting. WK1 doesn't even try to load the PDF document? I can reproduce locally if this test through WK1... I'm thinking of just skipping on WK1, given that there is coverage on WK2.

[Joseph Pecoraro]

(In reply to comment #17)
> (In reply to comment #15)
> > Comment on attachment 255223 [details]
> > [PATCH] Proposed Fix - Alternate Test Approach
> > 
> > Attachment 255223 [details] did not pass mac-ews (mac):
> > Output: http://webkit-queues.appspot.com/results/5704227661807616
> > 
> > New failing tests:
> > http/tests/security/mixedContent/insecure-content-in-image-document.html
> 
> Ahh interesting. WK1 doesn't even try to load the PDF document? I can
> reproduce locally if this test through WK1... I'm thinking of just skipping
> on WK1, given that there is coverage on WK2.

Ugh, now I'm able to reproduce if I just repeat the test for multiple iterations only the first succeed and the rest fail...

Is there any reliable way to load a PDF in tests?

[Joseph Pecoraro]

> Ugh, now I'm able to reproduce if I just repeat the test for multiple
> iterations only the first succeed and the rest fail...

Since even this test is unreliable when run multiple times in WK2, I'm going to drop the test... I've spent too much time trying to get this test to work given the simplicity of the fix, and that is has been reproduced and verified. =(

[Joseph Pecoraro]

Created attachment 255241 [details]
[PATCH] For Landing - Without Test

[WebKit Commit Bot]

Comment on attachment 255241 [details]
[PATCH] For Landing - Without Test

Clearing flags on attachment: 255241

Committed r185781: <http://trac.webkit.org/changeset/185781>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.