Bug 60559 – Frequent crashes beneath WebCore::ScriptElement::prepareScript

• Component: DOM
• Status: RESOLVED CONFIGURATION CHANGED

[Sam Weinig]

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.

[Sam Weinig]

<rdar://problem/9277712>

[Tony Gentilcore]

Sam, do you have a rough idea of when this started to appear?

[Sam Weinig]

We first started getting reports mid-april.

[Sam Weinig]

In all the crashlogs it is always beneath doccument.write.

[Eric Seidel (no email)]

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.

[Adam Barth]

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.

[Sam Weinig]

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).

[Eric Seidel (no email)]

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.

[Sam Weinig]

(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.

[Adam Barth]

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.

[Sam Weinig]

Landed work around in r86270.

[Tony Gentilcore]

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.

[Ademar Reis]

Revision r86270 cherry-picked into qtwebkit-2.2 with commit 3374ed3 <http://gitorious.org/webkit/qtwebkit/commit/3374ed3>

[Joseph Pecoraro]

This ASSERT has been in for years. Maybe we found some cases and they have been addressed over time?

[Anne van Kesteren]

As per comment 14.