Bug 40742 – WebCore crashes when removing a link element in a beforeload handler

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[Andy Estes]

Created attachment 58941 [details]
test case

If a page has a beforeload handler that removes a stylesheet <link> element, WebCore will crash with the following backtrace:

Crash is in WebCore::Node::createRendererIfNeeded:

0   com.apple.WebCore             	0x00007fff8808d6bd WebCore::Node::createRendererIfNeeded() + 45
1   com.apple.WebCore             	0x00007fff8808d5a0 WebCore::Element::attach() + 32
2   com.apple.WebCore             	0x00007fff8808cbb1 WebCore::HTMLParser::insertNode(WebCore::Node*, bool) + 369
3   com.apple.WebCore             	0x00007fff88110fd4 WebCore::HTMLParser::parseToken(WebCore::Token*) + 868
4   com.apple.WebCore             	0x00007fff8808c621 WebCore::HTMLTokenizer::processToken() + 657
5   com.apple.WebCore             	0x00007fff8810d786 WebCore::HTMLTokenizer::parseTag(WebCore::SegmentedString&, WebCore::HTMLTokenizer::State) + 4950
6   com.apple.WebCore             	0x00007fff8810bc60 WebCore::HTMLTokenizer::write(WebCore::SegmentedString const&, bool) + 2720
7   com.apple.WebCore             	0x00007fff8826607e WebCore::HTMLTokenizer::executeExternalScriptsIfReady() + 1694
8   com.apple.WebCore             	0x00007fff882147fc WebCore::CachedScript::checkNotify() + 76
9   com.apple.WebCore             	0x00007fff881e9226 WebCore::Loader::Host::didFinishLoading(WebCore::SubresourceLoader*) + 358
10  com.apple.WebCore             	0x00007fff881e9021 WebCore::SubresourceLoader::didFinishLoading() + 49

The attached .html file demonstrates the crash.

[Andy Estes]

<rdar://problem/8091385>

[Andy Estes]

Created attachment 59074 [details]
Patch

Comment on attachment 59074 [details]
Patch

> +        Postpone loading of link elements until after they have been inserted into the DOM and
> +        attached. This prevents mutation events triggered by beforeload handlers from firing in the
> +        midst of DOM insertion, which can lead to assertion failures and crashes.

Is the problem here really the mutation events, or merely the fact that the beforeload handler mutates the DOM during insertion of the link element?

> +void HTMLLinkElement::processCallback(Node* n)
> +{
> +    static_cast<HTMLLinkElement*>(n)->process();
> +}

I would assert (or ASSERT_ARG) that n is a link element before casting it.

> +	<link rel="stylesheet" href="">

href="" means that the base URL (the .html file in this case) will be loaded as the stylesheet in this case. Not a big deal, but perhaps href="data:text/css," is cleaner (if it doesn’t invalidate the test).

[Andy Estes]

(In reply to comment #3)
> (From update of attachment 59074 [details])
> > +        Postpone loading of link elements until after they have been inserted into the DOM and
> > +        attached. This prevents mutation events triggered by beforeload handlers from firing in the
> > +        midst of DOM insertion, which can lead to assertion failures and crashes.
> 
> Is the problem here really the mutation events, or merely the fact that the beforeload handler mutates the DOM during insertion of the link element?

You're right, "mutation event" is the wrong term to use.  I'll reword that before landing.

> 
> > +void HTMLLinkElement::processCallback(Node* n)
> > +{
> > +    static_cast<HTMLLinkElement*>(n)->process();
> > +}
> 
> I would assert (or ASSERT_ARG) that n is a link element before casting it.

Will do.

> 
> > +	<link rel="stylesheet" href="">
> 
> href="" means that the base URL (the .html file in this case) will be loaded as the stylesheet in this case. Not a big deal, but perhaps href="data:text/css," is cleaner (if it doesn’t invalidate the test).

Ahh, I didn't realize that.  I'll change that and re-test before landing.

Thanks for the review!

[Darin Adler]

Comment on attachment 59074 [details]
Patch

WebCore/html/HTMLLinkElement.cpp:234
 +  void HTMLLinkElement::processCallback(Node* n)
We typically use words for variable and argument names, not letters. So "node" would be preferred over "n".

[Andy Estes]

http://trac.webkit.org/changeset/61424

[Eric Seidel (no email)]

Test appears to fail on Tiger.

[Andy Estes]

(In reply to comment #7)
> Test appears to fail on Tiger.

The only test I see failing on Tiger is platform/mac/accessibility/inherited-presentational-lists.html.

[Eric Seidel (no email)]

@ap: this seems like another symptom of StyleSheet's bad node-ownership design.  If you fix that, we may be able to remove this code.