[Webkit-unassigned] [Bug 40764] editing/selection/leave-requested-block.html crash - maybe HTML5 related

Thu Jun 17 16:51:07 PDT 2010

https://bugs.webkit.org/show_bug.cgi?id=40764

--- Comment #6 from Eric Seidel <eric at webkit.org>  2010-06-17 16:51:07 PST ---
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.