| Summary: | Frequent crashes beneath WebCore::ScriptElement::prepareScript | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Sam Weinig <sam> | ||||||
| Component: | DOM | Assignee: | Tony Gentilcore <tonyg> | ||||||
| Status: | RESOLVED CONFIGURATION CHANGED | ||||||||
| Severity: | Normal | CC: | abarth, ademar, aestes, annevk, ap, eric, joepeck, simonjam, tonyg | ||||||
| Priority: | P2 | Keywords: | InRadar | ||||||
| Version: | 528+ (Nightly build) | ||||||||
| Hardware: | All | ||||||||
| OS: | All | ||||||||
| Attachments: |
|
||||||||
Sam, do you have a rough idea of when this started to appear? We first started getting reports mid-april. In all the crashlogs it is always beneath doccument.write. I could see how this could happen. We need a bunch more ASSERTs around things like takeScriptToProcess to make sure its' never being called when m_scriptToProcess is 0. I don't know the exact series of steps which could cause this, but it's definitely possible to call takeScriptToProcess at the wrong time. I suspect that some part of code is seeing the tree builder paused and assuming its for scripts and we're getitng in here. not sure. HTMLDocumentParser::runScriptsForPausedTreeBuilder assumes m_treeBuilder->isPaused() implies m_treeBuilder->takeScriptToProcess(scriptStartPosition) is non-null, but HTMLScriptRunner::execute returning false can make m_treeBuilder->isPaused() stay true even though the element has been taken.
In HTMLScriptRunner::execute this code looks related to the nesting:
if (haveParsingBlockingScript()) {
if (m_scriptNestingLevel)
return false; // Block the parser. Unwind to the outermost HTMLScriptRunner::execute before continuing parsing.
So... Maybe if you document.write an inline script followed by something else that gets us running down these code paths. Not sure.
Created attachment 93161 [details]
Patch to paper over the issue.
I would like to land this patch to fix the crash at the source, but leave in the ASSERT and this bug open, to try and solve it in a more robust way (and hopefully find a test case).
Comment on attachment 93161 [details]
Patch to paper over the issue.
I think we want an earlier assert. I mean, this patch isn't bad, but it just papers over the crash.
(In reply to comment #8) > (From update of attachment 93161 [details]) > I think we want an earlier assert. I mean, this patch isn't bad, but it just papers over the crash. I can certainly add an earlier assert, perhaps in HTMLDocumentParser::runScriptsForPausedTreeBuilder, but the intent of this patch is to paper over the issue to avoid the crash in the meantime. Comment on attachment 93161 [details]
Patch to paper over the issue.
I think this is fine for now, but I'll let Eric do the official review. Please leave the bug open so we can write the proper fix later.
Landed work around in r86270. In notifyFinished() and executeScriptsWaitingForStylesheets() we un-pause immediately before script execution, but in runScriptsForPausedTreeBuilder() we leave it paused during execution. It looks like we should always unpause before execution. I'll put together a repo and patch. Revision r86270 cherry-picked into qtwebkit-2.2 with commit 3374ed3 <http://gitorious.org/webkit/qtwebkit/commit/3374ed3> This ASSERT has been in for years. Maybe we found some cases and they have been addressed over time? As per comment 14. |
Created attachment 92975 [details] Crash Log We are seeing a high volume of crashes under WebCore::ScriptElement::prepareScript, unfortunately, I have not found any repro steps yet. It looks to me like there is a null ScriptElement in HTMLScriptRunner::runScript, though I have not worked out yet how that could happen. Attaching sample crash log.