471166 - Don't let EndUpdate run script while there are scriptblockers

Status: RESOLVED FIXED

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

Description

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

Bug 466057 made it more catastrophic to run script while there are scriptblockers. However one place where that is currently likely to happen is in the various EndUpdate implementations. We need to either make sure that we never enter updates while there are scriptblockers, or ensure that BeginUpdate/EndUpdate doesn't directly run scripts.

See also bug 423269

Comment 1

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

Since 466057 has landed on the 1.9.1 branch, we need to fix this there too.

Smaug: Do you have the cycles? If not I can do it.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

I can do this.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Attachment: wip1

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Attachment: wip1.1

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 357379 [details] [diff] [review]
wip1.1

Jonas, what you think about this?
I could perhaps try to make some macro "RUN_AFTER_UPDATE_WHEN_SAFE", but frameloaders and xbl needs anyway special handling.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Other option might be to postpone the outermost EndUpdate until there are no script blockers, but that would sort of break symmetric BeginUpdate-EndUpdate.

Comment 7

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

Your current patch makes one behavioral change to how we run XBL ctors. Given the following order of events:

1. First scriptblocker is added
2. First update starts
3. Last update ends
4. First update starts
5. Last update ends
6. Last scriptblocker is removed

Right now bindings attached between 2 and 3 get their ctor run at 3. Then bindings attached between 4 and 5 get their ctor run at 5. Bindings attached between 1-2, 3-4 and 5-6 are run much later from an event off the main event loop.

With your patch, any bindings attached between 2 and 6 get their ctor run at 6. And they are run in reverse order (mAttached*Stack*) which means that bindings attached between 4-5 are run before bindings at 2-3.

I do however think this is fine. Or rather, I can't see any other way to do it.


So here's an alternative way to deal with XBL:

1. Remove nsBindingManager::Begin/EndOutermostUpdate, including the calls
   to them in nsDocument
2. In AddToAttachedQueue:
   * If there are scriptblockers, schedule a scriptrunner, unless one is
     already scheduled.
   * If there aren't scriptblockers, call PostProcessAttachedQueueEvent.
3. When the scriptrunner runs, call ProcessAttachedQueue
4. In ProcessAttachedQueue, grab the current mAttachedStackSizeOnOutermost and
   store in a local variable.
5. Pop bindings of the end of mAttachedStack and for each binding set
   mAttachedStackSizeOnOutermost to the current mAttachedStack length and then
   run the binding ctor.

You can use the same scriptrunner each time to avoid creating lots of scriptrunners.

The result of this is that all bindings attached between 1 and 6 (in the top list of events) are run at 6.

However the more I think about this, the more I think we should go with your patch for now. We can always do something else later, or just do something better for XBL2, which should define how all of this works.

Comment 8

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

Specifically why I don't think we should go with what comment 7 says is that I think layout often does:

1. Start scriptblocker
2. Add bindings
3. End scriptblocker

with your patch the above behaves the same. With my strategy we'd run all the binding ctors at 3.

Comment 9

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

Comment on attachment 357379 [details] [diff] [review]
wip1.1

>diff --git a/content/base/src/nsDocument.cpp b/content/base/src/nsDocument.cpp
>--- a/content/base/src/nsDocument.cpp
>+++ b/content/base/src/nsDocument.cpp
>@@ -3664,19 +3664,37 @@ nsDocument::RemoveObserver(nsIDocumentOb
>     RemoveMutationObserver(aObserver);
>     return mObservers.RemoveElement(aObserver);
>   }
> 
>   return mObservers.Contains(aObserver);
> }
> 
> void
>+nsDocument::MaybeEndOutermostXBLUpdate()
>+{
>+  // Only call BindingManager()->EndOutermostUpdate() when
>+  // we're not in an update and it is safe to run scripts.
>+  if (mUpdateNestLevel == 0 && mInXBLUpdate) {
>+    if (nsContentUtils::IsSafeToRunScript()) {
>+      mInXBLUpdate = PR_FALSE;
>+      BindingManager()->EndOutermostUpdate();
>+    } else if (!mInDestructor) {
>+      nsRefPtr<nsRunnableMethod<nsDocument> > xblEndUpdateEvent =
>+        NS_NEW_RUNNABLE_METHOD(nsDocument, this, MaybeEndOutermostXBLUpdate);
>+      nsContentUtils::AddScriptRunner(xblEndUpdateEvent);

You can change this general pattern to simply

nsContentUtils::AddScriptRunner(NS_NEW_RUNNABLE_METHOD(nsDocument, this, MaybeEndOutermostXBLUpdate));

I commented in the AddScriptRunner declaration that that is ok.

Same thing in nsXULDocument::MaybeBroadcast


Did you also check all implementations of nsIDocumentObserver::EndUpdate to make sure they don't run scripts?

r/sr=me if so.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #9)
> Did you also check all implementations of nsIDocumentObserver::EndUpdate to
> make sure they don't run scripts?
I did and couldn't see anything problematic.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Attachment: patch

http://hg.mozilla.org/mozilla-central/rev/9e71c5606d11

Comment 12

[Boris Zbarsky [:bzbarsky]]

> nsContentUtils::AddScriptRunner(NS_NEW_RUNNABLE_METHOD(nsDocument, this,
> MaybeEndOutermostXBLUpdate));

How often do we call this code?  Is the allocation cost going to matter?

Comment 13

[Gordon P. Hemsley [:GPHemsley]]

This line could use some more parentheses to disambiguate what is meant:
http://hg.mozilla.org/mozilla-central/rev/9e71c5606d111140867e8de98444e3e7cc6e9802#l2.9

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

Attachment: 1.9.0 patch

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

Attachment: +missing #include

Comment 18

[Daniel Veditz [:dveditz]]

Comment on attachment 360075 [details] [diff] [review]
+missing #include

Approved for 1.9.0.7, a=dveditz for release-drivers.

Comment 19

[Daniel Veditz [:dveditz]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/03fee6722a1f

Comment 20

[Daniel Veditz [:dveditz]]

Comment on attachment 360074 [details] [diff] [review]
1.9.0 patch

Approved for 1.9.0.7, a=dveditz for release-drivers.

Comment 21

[Daniel Veditz [:dveditz]]

No scriptblockers in 1.8

Comment 22

[Al Billings [:abillings - ex-MoCo]]

Is there any way to directly test this in 1.9.0.7? I notice that there is no unit test for this.

Comment 23

[Mike Beltzner [:beltzner, not reading bugmail]]

Mass change: adding fixed1.9.2 keyword

(This bug was identified as a mozilla1.9.2 blocker which was fixed before the mozilla-1.9.2 repository was branched (August 13th, 2009) as per this query: http://is.gd/2ydcb - if this bug is not actually fixed on mozilla1.9.2, please remove the keyword. Apologies for the bugspam)