Bug 40764 – REGRESSION(HTML5 parser): editing/selection/leave-requested-block.html can fail or crash

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

[Nikolas Zimmermann]

Just ran DRT (old-run-webkit-tests) and it gave me:
editing/selection/leave-requested-block.html -> crashed

Exception Type:  EXC_BAD_ACCESS (SIGSEGV)
Exception Codes: KERN_INVALID_ADDRESS at 0x00000000bbadbeef
Crashed Thread:  0

Thread 0 Crashed:
0   com.apple.WebCore             	0x04209531 WebCore::CachedResource::addClientToSet(WebCore::CachedResourceClient*) + 75 (CachedResource.cpp:209)
1   com.apple.WebCore             	0x042099e8 WebCore::CachedResource::addClient(WebCore::CachedResourceClient*) + 24 (CachedResource.cpp:204)
2   com.apple.WebCore             	0x0454c1c8 WebCore::CachedScriptSourceProvider::CachedScriptSourceProvider(WebCore::CachedScript*) + 180
3   com.apple.WebCore             	0x0454c1fb WebCore::CachedScriptSourceProvider::create(WebCore::CachedScript*) + 43 (CachedScriptSourceProvider.h:40)
4   com.apple.WebCore             	0x0454c434 WebCore::ScriptSourceCode::ScriptSourceCode(WebCore::CachedScript*) + 24
5   com.apple.WebCore             	0x0454b7f8 WebCore::HTML5ScriptRunner::sourceFromPendingScript(WebCore::HTML5ScriptRunner::PendingScript const&, bool&) + 190 (HTML5ScriptRunner.cpp:85)
6   com.apple.WebCore             	0x0454b9a9 WebCore::HTML5ScriptRunner::executePendingScript() + 295 (HTML5ScriptRunner.cpp:107)
7   com.apple.WebCore             	0x0454bbb6 WebCore::HTML5ScriptRunner::executeParsingBlockingScripts() + 56 (HTML5ScriptRunner.cpp:184)
8   com.apple.WebCore             	0x0454bcca WebCore::HTML5ScriptRunner::executeScriptsWaitingForStylesheets() + 248 (HTML5ScriptRunner.cpp:208)
9   com.apple.WebCore             	0x0454d618 WebCore::HTML5DocumentParser::executeScriptsWaitingForStylesheets() + 284 (HTML5DocumentParser.cpp:430)
10  com.apple.WebCore             	0x04382b87 WebCore::Document::removePendingSheet() + 201 (Document.cpp:2577)
11  com.apple.WebCore             	0x0459979c WebCore::HTMLLinkElement::sheetLoaded() + 106 (HTMLLinkElement.cpp:330)
12  com.apple.WebCore             	0x04329e19 WebCore::CSSStyleSheet::checkLoaded() + 137 (CSSStyleSheet.cpp:213)
13  com.apple.WebCore             	0x0459adc7 WebCore::HTMLLinkElement::setCSSStyleSheet(WebCore::String const&, WebCore::KURL const&, WebCore::String const&, WebCore::CachedCSSStyleSheet const*) + 1327 (HTMLLinkElement.cpp:314)
14  com.apple.WebCore             	0x04202002 WebCore::CachedCSSStyleSheet::checkNotify() + 176 (CachedCSSStyleSheet.cpp:116)
15  com.apple.WebCore             	0x042021df WebCore::CachedCSSStyleSheet::data(WTF::PassRefPtr<WebCore::SharedBuffer>, bool) + 385 (CachedCSSStyleSheet.cpp:106)
16  com.apple.WebCore             	0x0497aefa WebCore::Loader::Host::didFinishLoading(WebCore::SubresourceLoader*) + 464 (loader.cpp:406)
17  com.apple.WebCore             	0x04c22286 WebCore::SubresourceLoader::didFinishLoading() + 176 (SubresourceLoader.cpp:196)
18  com.apple.WebCore             	0x04b7a670 WebCore::ResourceLoader::didFinishLoading(WebCore::ResourceHandle*) + 24 (ResourceLoader.cpp:444)
19  com.apple.WebCore             	0x04b76aa9 -[WebCoreResourceHandleAsDelegate connectionDidFinishLoading:] + 215 (ResourceHandleMac.mm:862)
20  com.apple.Foundation          	0x96688497 -[NSURLConnection(NSURLConnectionReallyInternal) sendDidFinishLoading] + 87

I'm not sure if it's HTML5 related, but I thought it's a good idea to let you guys know.

[Eric Seidel (no email)]

Bug 40754 is likely the same issue.

We've seen this ASSERT before.  Somehow we're trying to ref a dead CachedResource node.  This may be related to Tony's proposed CachedResourceHandler fix.

[Tony Gentilcore]

I think this bug is different(In reply to comment #1)
> Bug 40754 is likely the same issue.
> 
> We've seen this ASSERT before.  Somehow we're trying to ref a dead CachedResource node.  This may be related to Tony's proposed CachedResourceHandler fix.

I think this bug is different. My patch was in the HTMLDocumentParser which is now unused.
http://trac.webkit.org/changeset/61306

This stack is in the HTML5DocumentParser/ScriptRunner.

[Eric Seidel (no email)]

Looking at this now.

[Eric Seidel (no email)]

This is hitting:

void CachedResource::addClientToSet(CachedResourceClient* client)
{
    ASSERT(!isPurgeable());

Trying to add itself as a client to a cachedResource which is purgeable because it has no clients.

We would hit this case when we encounter a <script> tag which is already in our cache, but we can't execute it because a .css file is blocking it.

[Eric Seidel (no email)]

I need to find out how to make a CachedResource with an m_purgeableData.

[Eric Seidel (no email)]

OK.  So here is what's going on:

CachedResource::addClient has this strange (IMO) behavior of immediately calling notifyFinished() on the newly added client, if the resource is already loaded.  This can cause unexpected reentrancy.  Lots of places in the code do a dance around this.  HTML5ScriptRunner's chosen dance was not to call addClient() if the CachedResource was already fully loaded.

For simplicity, void HTML5ScriptRunner::requestScript(Element* script) always makes the newly requested script the m_parsingBlockingScript, whether it's about to run it or not.  When requestScript returns, the caller HTML5ScriptRunner::execute will execute any pending scripts (including the one we just requested).  This allows us to have one place for handling "don't run while loading CSS" as well as "unwrap to the outermost HTML5ScriptRunner::execute caller before executing any pending scripts" logic, as required by HTML5.

However, this don't-addClient-if-fully-loaded dance gets us in trouble, if we're waiting on CSS loads.  As we'll return from requestScript w/o calling addClient, and then when HTML5ScriptRunner::execute decides it can't execute any scripts right now due to waiting for a CSS load, then we return to the run loop w/o calling addClient.  This is fine, except that CachedResource decides if data is purgeable not by its ref count, but rather by its count of clients (which is still 0).

So once the CSS file finally loads, we go to execute the script.  ScriptSourceCode tries to sign itself up as a client (not sure why, but doesn't really matter), and if the data is gone, then in Release builds we'll execute an empty script, or in Debug builds we'll crash on the ASSERT that WildFox hit in this bug.

There are multiple fixes we could consider.  The "easiest" would be to add a hack to requestScript to addClient ourselves in the case where we're blocked on CSS loads.  That would cause immediate reentrancy through notifyFinished, but that would immediately exit again due to waiting on the CSS load.  That's very close to how the old parser would work in this situation, but is a total horribly grose hack.

Better would be to add a new flavor of "addClient" which doesn't notify and call it that instead.  There are probably lots of places in the code which want this.  The danger there is that such an addClient could get you into trouble as you would never be called if the resource was already loaded.  Normally it might be OK to ASSERT in such an addClientButDontNotify call that the resource wasn't loaded, but due to our current requestScript design, we'd hit that ASSERT.

Still considering my options.

[Eric Seidel (no email)]

*** Bug 40754 has been marked as a duplicate of this bug. ***

[Eric Seidel (no email)]

Created attachment 59058 [details]
Patch

[Eric Seidel (no email)]

Created attachment 59060 [details]
Patch

[Adam Barth]

Comment on attachment 59060 [details]
Patch

okiedokes

[Eric Seidel (no email)]

Created attachment 59065 [details]
After feedback from Adam

[Adam Barth]

Comment on attachment 59065 [details]
After feedback from Adam

thanks.  this is much nicer.  :)

[Eric Seidel (no email)]

Committed r61374: <http://trac.webkit.org/changeset/61374>