398222 - [FIX]Don't call ReadyToExecuteScripts() if nothing to execute

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P3)

Description

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I just crashed on a local copy of www.rediff.com with the following stack:

nsScriptLoader::ReadyToExecuteScripts()  Line 680 + 0x10 bytes C++
nsScriptLoader::ProcessPendingRequests()  Line 658 + 0x8 bytes C++
nsScriptLoader::RemoveExecuteBlocker()  Line 176 C++
nsDocument::EndUpdate(unsigned int aUpdateType=2)  Line 2702 C++
nsStyleLinkElement::DoUpdateStyleSheet(nsIDocument * aOldDocument=0x069b10e8, nsICSSLoaderObserver * aObserver=0x00000000, int * aWillNotify=0x0012ece0, int * aIsAlternate=0x0012ecdc, int aForceUpdate=0)  Line 233  C++
nsStyleLinkElement::UpdateStyleSheetInternal(nsIDocument * aOldDocument=0x069b10e8, int aForceUpdate=0)  Line 214  C++
nsHTMLLinkElement::UnbindFromTree(int aDeep=1, int aNullParent=0)  Line 252  C++
nsGenericElement::UnbindFromTree(int aDeep=1, int aNullParent=0)  Line 2133  C++
nsGenericHTMLElement::UnbindFromTree(int aDeep=1, int aNullParent=0)  Line 1178  C++

Not sure which step in this is the bad one though. Additionally it seems weird that the simple BeginUpdate/EndUpdate pair that nsStyleLinkElement::DoUpdateStyleSheet does will cause the scriptloader to start executing scripts. That should only happen if a pending script is added between the BeginUpdate and the EndUpdate, which shouldn't be the case here as the only thing happening is that a stylesheet is removed. I.e. the code is:

    aOldDocument->BeginUpdate(UPDATE_STYLE);
    aOldDocument->RemoveStyleSheet(mStyleSheet);
    aOldDocument->EndUpdate(UPDATE_STYLE);

which I would have thought should be fine.

Comment 1

[Boris Zbarsky [:bzbarsky]]

We're not actually executing scripts, necessarily.  Consider the following sequence of events, which is what I think is happening here:

1)  AddExecuteBlocker() is called from BeginUpdate().  mPendingRequests.Count()
    is 0, so we set mHadPendingScripts false.
2)  RemoveExecuteBlocker() is called from EndUpdate.  mHadPendingScripts is
    false, so we call ProcessPEndingRequests (and mPendingRequests.Count() is
    still 0).
3)  ReadyToExecuteScripts() is called (since we call that before checking
    mPendingRequests.Count()).
4)  The GetParentDocumet() chain is bogus (which I think you ran into with the
    nsDocument::Destroy change, right?)
5)  We crash on line 680 as in the above stack, trying to get a ScriptLoader()
    off a bogus pointer.

Perhaps we should not bother with the ReadyToExecuteScripts() check if we have nothing to execute, since it's actually a little expensive now?  In any case, this doesn't look like a problem to me, other than the busted parent document chain.

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Yup, I think you are absolutely right. Extra points for the mind-reading trick of figuring out that this was happening with the nsDocument::Destroy patch :)

I think you're right though, we should probably avoid calling ReadyToExecuteScripts if we don't have any pending scripts. Given that we currently do every time Begin/EndUpdate nesting reaches zero.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Want to patch?  Or should I?  I'm not sure when I'll be able to check in, to be honest....

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Like so

Comment 5

[Boris Zbarsky [:bzbarsky]]

Checked in.  Nothing to test here.