Bug 60559 - Frequent crashes beneath WebCore::ScriptElement::prepareScript
Summary: Frequent crashes beneath WebCore::ScriptElement::prepareScript
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Tony Gentilcore
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2011-05-10 10:33 PDT by Sam Weinig
Modified: 2016-12-13 13:56 PST (History)
8 users (show)

See Also:


Attachments
Crash Log (6.21 KB, text/plain)
2011-05-10 10:33 PDT, Sam Weinig
no flags Details
Patch to paper over the issue. (1.65 KB, patch)
2011-05-11 12:46 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sam Weinig 2011-05-10 10:33:28 PDT
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.
Comment 1 Sam Weinig 2011-05-10 10:34:52 PDT
<rdar://problem/9277712>
Comment 2 Tony Gentilcore 2011-05-10 10:40:42 PDT
Sam, do you have a rough idea of when this started to appear?
Comment 3 Sam Weinig 2011-05-10 10:52:58 PDT
We first started getting reports mid-april.
Comment 4 Sam Weinig 2011-05-10 10:58:28 PDT
In all the crashlogs it is always beneath doccument.write.
Comment 5 Eric Seidel 2011-05-10 11:02:37 PDT
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.
Comment 6 Adam Barth 2011-05-10 15:24:21 PDT
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.
Comment 7 Sam Weinig 2011-05-11 12:46:33 PDT
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 8 Eric Seidel 2011-05-11 13:21:52 PDT
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.
Comment 9 Sam Weinig 2011-05-11 13:29:26 PDT
(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 10 Adam Barth 2011-05-11 13:31:35 PDT
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.
Comment 11 Sam Weinig 2011-05-11 14:18:24 PDT
Landed work around in r86270.
Comment 12 Tony Gentilcore 2011-05-16 06:25:23 PDT
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.
Comment 13 Ademar Reis 2011-05-26 15:44:38 PDT
Revision r86270 cherry-picked into qtwebkit-2.2 with commit 3374ed3 <http://gitorious.org/webkit/qtwebkit/commit/3374ed3>
Comment 14 Joseph Pecoraro 2016-12-13 13:56:29 PST
This ASSERT has been in for years. Maybe we found some cases and they have been addressed over time?