453736 - [FIX]Crash under nsSVGElement::BindToTree with <svg:use>, <svg:script>

Status: VERIFIED FIXED

(Core :: SVG, defect, P1)

Description

[Jesse Ruderman]

Attachment: testcase (crashes Firefox within a minute)

Steps to reproduce:
1. Load the testcase from a file: URL.
2. Wait 5-60 seconds.

Result: Some kind of crash under nsSVGElement::BindToTree.  It may involve calling a bogus memory address.

You'll probably also see some of these assertions:

###!!! ASSERTION: No callbacks should be registered while we set up this entry: '!entry->HasContentChangeCallback()', file /Users/jruderman/central/content/base/src/nsDocument.cpp, line 3288

###!!! ASSERTION: Please remove this from the document properly: '!IsInDoc()', file /Users/jruderman/central/content/base/src/nsGenericElement.cpp, line 1743

###!!! ASSERTION: Already have a document.  Unbind first!: '!GetCurrentDoc() && !IsInDoc()', file /Users/jruderman/central/content/base/src/nsGenericDOMDataNode.cpp, line 585

###!!! ASSERTION: Shallow unbind won't clear document and binding parent on kids!: 'aDeep || (!GetCurrentDoc() && !GetBindingParent())', file /Users/jruderman/central/content/base/src/nsGenericElement.cpp, line 2730

###!!! ASSERTION: Bound to wrong document: 'aDocument == GetCurrentDoc()', file /Users/jruderman/central/content/base/src/nsGenericElement.cpp, line 2717

###!!! ASSERTION: Bound to wrong parent: 'aParent == GetParent()', file /Users/jruderman/central/content/base/src/nsGenericElement.cpp, line 2718

The testcase is loosely based on dynamic-use-01.svg, one of the most evil reftests in the Mozilla tree.

Comment 1

[Robert Longson [:longsonr]]

Attachment: patch?

I couldn't get it to crash but I did see various assertions.

Does this fix the crash for you?

Comment 2

[Jesse Ruderman]

That made some of the assertions go away, but I still get

###!!! ASSERTION: Bound to wrong parent: 'aParent == GetParent()', file /Users/jruderman/central/content/base/src/nsGenericElement.cpp, line 2724

and a scary crash.

Comment 3

[Samuel Sidler (old account; do not CC)]

Jess, does this affect the 1.9.0 branch?

Robert, any update here?

Comment 4

[Robert Longson [:longsonr]]

Sorry, I couldn't figure out how to fix it. I knew that patch? was incorrect but comment 2 showed I wasn't even in the right ball park.

The 1.9.0 code is totally different so its unlikely to be affected but then I couldn't get the 1.9.1 code to crash (just assert lots) so I guess the scary bit  might well be mac only.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix

Two things going on here.  First of all, nsSVGScriptElement::Clone has never been implemented correctly.  In particular it never copied the mIsEvaluaed state.  Second, the fix for bug 343730 didn't remove the members that shadow nsIScriptElement.

So without this patch what happens in some cases is that when the <use> frame is constructed we clone the script, bind it to tree, and immediately execute it (during frame construction!).

This should be simple to trigger by toggling the <use> from display:none to a visible display value, I bet.  I'll see what I can do about creating a crashtest, or even just mochitest (or both), based on that if Jesse doesn't get to it first.

Comment 6

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

Comment on attachment 345442 [details] [diff] [review]
Fix

This looks good in and of itself, but i'm less sure this is enough to fix the bug.

Isn't there a risk that we'll end up cloning a unevaluated script somehow? Actually, it looks like if there is a <noframes> element somewhere up the parent chain the script will never evaluate or get marked as evaluated.

We should probably make ns{HTML|SVG}ScriptElement do something more drastic if there aren't any scriptblockers when we're inside BindToTree?

We might also want to add scriptblockers while <use> is calling BindToTree (which is what I assume is happening here?)

Comment 7

[Boris Zbarsky [:bzbarsky]]

> Isn't there a risk that we'll end up cloning a unevaluated script somehow?

Not with <use>, as far as I can see.

> Actually, it looks like if there is a <noframes> element somewhere up the
> parent chain the script will never evaluate or get marked as evaluated.

How so?  In HTML it won't even get parsed into an HTMLScriptElement.  In XHTML I think it'll just run, no?

> We should probably make ns{HTML|SVG}ScriptElement do something more drastic if
> there aren't any scriptblockers when we're inside BindToTree?

We could also do that, sure.  I think we need the patch in this bug as a bare minimum.

> We might also want to add scriptblockers while <use> is calling BindToTree
> (which is what I assume is happening here?)

<use> is not.  The frame constructor code that calls into nsIAnonymousContentCreator is.

Comment 8

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

(In reply to comment #7)
> > Isn't there a risk that we'll end up cloning a unevaluated script somehow?
> 
> Not with <use>, as far as I can see.
> 
> > Actually, it looks like if there is a <noframes> element somewhere up the
> > parent chain the script will never evaluate or get marked as evaluated.
> 
> How so?  In HTML it won't even get parsed into an HTMLScriptElement.  In XHTML
> I think it'll just run, no?

Not if you build the DOM using appendChild etc. See
http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsScriptElement.cpp#148

> > We might also want to add scriptblockers while <use> is calling BindToTree
> > (which is what I assume is happening here?)
> 
> <use> is not.  The frame constructor code that calls into
> nsIAnonymousContentCreator is.

Shouldn't the frame ctor code have installed scriptblockers while doing this then?

Though on second thought in general I don't think we ever want to execute scripts inside a <use>, so simply adding scriptblockers isn't what we want.

Comment 9

[Boris Zbarsky [:bzbarsky]]

> Not if you build the DOM using appendChild etc

Ugh.  I agree; we should fix that.  You want me to roll it into this patch?

> Shouldn't the frame ctor code have installed scriptblockers while doing this
> then?

Possible...  This is happening under restyle processing, so maybe we should install a scriptblocker around that whole loop, independently of what we do with <use>?

Comment 10

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

(In reply to comment #9)
> > Not if you build the DOM using appendChild etc
> 
> Ugh.  I agree; we should fix that.  You want me to roll it into this patch?

Please do

> > Shouldn't the frame ctor code have installed scriptblockers while doing this
> > then?
> 
> Possible...  This is happening under restyle processing, so maybe we should
> install a scriptblocker around that whole loop, independently of what we do
> with <use>?

Same here.

Comment 11

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

Comment on attachment 345442 [details] [diff] [review]
Fix

Removing request while waiting for new patch

Comment 12

[Boris Zbarsky [:bzbarsky]]

Attachment: Address review comments

This makes the following changes:

1)  Set mIsEvaluated if we bail out because of a non-scripting container.
2)  Fix the SVG cloning bug.
3)  Add/remove script blocker around the whole body of ProcessPendingRestyles.
4)  Add/remove script blockers in the Begin/EndUpdate in CSSFC.

Part 4 is mostly for safety, since those methods must be called whenever we twiddle the frame tree, and it's good to have script blocked at those points.  I still did #3 just so we don't run the script (if any) partway though ProcessPendingRestyles.

Note that the various frame construction code already asserts that we're in an update, so this means we'll now also be blocking scripts in it in all cases.  That's good.  ;)

This does mean that in some cases we'll block mutation events in anon content when we didn't use to, but I think we want that change.  It should be reasonably safe, even for branches, right?

Comment 13

[Daniel Veditz [:dveditz]]

I don't crash nor see the comment 0 assertions in 1.9.0.5

Comment 14

[Boris Zbarsky [:bzbarsky]]

Daniel, it should be pretty simple to modify the testcase to assert and crash on 1.9.0, I would think.  I was going to request branch approvals the second after I landed this on trunk.  This needs to happen on branches.

Comment 15

[Samuel Sidler (old account; do not CC)]

Jesse, can you work up a modified testcase that affects 1.9.0.x?

Comment 16

[Boris Zbarsky [:bzbarsky]]

I recommend something where the script sets everything in sight display:none.  That should get you nice deleted object virtual function calls.  You'll have to unset that in the main testcase to get the <use> to create frames, of course.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Attachment: Comment fix that should land with the other patch.

Comment 18

[Brandon Sterne (:bsterne)]

Hey Jonas, this bug is on our Top Security Bugs list, so can you provide review on Boris' patch when you get some time?  Thanks!

Comment 19

[Boris Zbarsky [:bzbarsky]]

With the expanded changes Jonas asked for, this needs to go into b3, in my opinion.

Comment 20

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

Comment on attachment 352740 [details] [diff] [review]
Address review comments

> nsCSSFrameConstructor::ProcessPendingRestyles()
> {
>   NS_PRECONDITION(mDocument, "No document?  Pshaw!\n");
> 
>+  // Do this out here so that we don't have to worry about it being
>+  // removed before our style data rebuild.
>+  nsAutoScriptBlocker scriptBlocker;

I think Smaug has added a scriptblocker around each callsite to this function, as well as an assertion to make sure that there are scriptblockers. So this change shouldn't be needed any more.

r/sr=me with that fixed.

Comment 21

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

Comment on attachment 352740 [details] [diff] [review]
Address review comments

> nsCSSFrameConstructor::BeginUpdate() {
>   NS_SuppressFocusEvent();
>+  nsContentUtils::AddScriptBlocker();
>   ++mUpdateCount;
> }

> nsCSSFrameConstructor::EndUpdate()
> {
>   if (mUpdateCount == 1) {
>     // This is the end of our last update.  Before we decrement
>     // mUpdateCount, recalc quotes and counters as needed.
> 
>     RecalcQuotesAndCounters();
>     NS_ASSERTION(mUpdateCount == 1, "Odd update count");
>   }
>+  // Do this before we unsuppress focus
>+  nsContentUtils::RemoveScriptBlocker();
>   NS_UnsuppressFocusEvent();
>   --mUpdateCount;
> }

Actually, why are these needed? Seems like it'll break mutation events

Comment 22

[Boris Zbarsky [:bzbarsky]]

Hmm.  You're right that those might break mutation events (do we have any testcases for this?)...  They're needed because there are Begin/EndUpdate callers in the frame constructor that are not tied to content updates and that are therefore not in an existing script blocker.

Comment 23

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

Actually, that might be an excellent reason not to add blockers there :)

There is at least one place [1] that can end up calling BeginUpdate without calling EndUpdate, something that would be very bad if BeginUpdate added a scriptblocker.

Though maybe things are equally bad already?

[1] nsCSSFrameConstructor::LazyGenerateChildrenEvent::Run

Comment 24

[Boris Zbarsky [:bzbarsky]]

Uh, yes.  That unbalanced begin without an end would break things badly.  If nothing else, it would break focus.  Please file a bug on that?

Comment 25

[Boris Zbarsky [:bzbarsky]]

For my reference, smaug's change to add script blockers around ProcessPendingRestyles is in bug 436965.  He did NOT add an assertion in ProcessPendingRestyles, but I'll do so.

As I was documenting the invariants here and adding assertions I found some more callers of some of this stuff that should have had script blockers but did not.  I'll add them.

Comment 26

[Boris Zbarsky [:bzbarsky]]

Er, I was wrong.  There was a script blocker assert in ProcessPendingRestyles, but not in ProcessRestyledFrames, which had some non-script-blocking entry points.

Comment 27

[Boris Zbarsky [:bzbarsky]]

OK.  So basically, smaug's patch for bug 453736 obviated the need for most of the script blocker stuff in my patch.  I looked through all the Begin/EndUpdate callers in the frame constructor, and all have script blockers except the one in ProcessRestyledFrames coming from RebuildAllStyleData (added script blocker and assert) and the one in listbox code, which smaug has a patch for elsewhere anyway, as I recall.

So the patch coming up is just items 1 and 2 from comment 12, plus the RebuildAllStyleData change.

I do think we should land this on all of m-c, 1.9.1, 1.9.0.  I'll probably try to write a 1.8 patch for just item 2 (and maybe item 1) from comment 12 as well.

Comment 28

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch

I fixed the Run() thing while I was here.

Comment 29

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

Comment on attachment 356884 [details] [diff] [review]
Patch

r/sr=me, though feel free to get someone else to look at the viewbatch stuff as I don't really know that code.

Comment 30

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 356884 [details] [diff] [review]
Patch

roc, want to double-check the view batch bit?

Comment 31

[Boris Zbarsky [:bzbarsky]]

Attachment: 1.9.1 version

Comment 32

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/mozilla-central/rev/b11d1f574c32 and http://hg.mozilla.org/releases/mozilla-1.9.1/rev/42e0d4b2c804

Comment 33

[Jesse Ruderman]

in-testsuite+: patch contains a mochitest, original testcase is too slow to be a crashtest

Comment 34

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Verified based on tinderbox status on trunk and 1.9.1 branch.

Comment 35

[Samuel Sidler (old account; do not CC)]

Boris: Can you write a 1.8 branch patch for this (per comment 27)?

Comment 36

[Daniel Veditz [:dveditz]]

We'll take this on the 1.8 branch when it's fixed on 1.9.0, not blocking for the current 1.8.1.21 release (which has rough parity to the code-frozen 1.9.0.7 release).

Comment 37

[Boris Zbarsky [:bzbarsky]]

Er, I meant smaug's patch for bug 436965 in comment 27.

Backporting this to 1.9.0 depended on that change, but now that's happened.  I'll try to post 1.9.0 and 1.8.1 patches ASAP.

Comment 38

[Samuel Sidler (old account; do not CC)]

Boris: Does that mean we need to take bug 436965 on the 1.8.1 branch?

Comment 39

[Boris Zbarsky [:bzbarsky]]

No; I'm just going to make the 1.8.1 patch have slightly smaller scope.  No scriptblockers at all on 1.8.1, iirc, so no need to worry about managing them.

Comment 40

[Boris Zbarsky [:bzbarsky]]

Attachment: 1.8 branch fix

Comment 41

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

Wait, don't you need to worry about the problem that if the <script> has a <noXXX> container we won't run it in the main document, but will run it inside the <svg:use> clone?

Comment 42

[Boris Zbarsky [:bzbarsky]]

We don't do noXXX checks on clones for HTML either, on 1.8 branch.  In fact, I don't think we do them period.  So no, I'm not going to worry about it.

Comment 43

[Daniel Veditz [:dveditz]]

Comment on attachment 364373 [details] [diff] [review]
1.8 branch fix

1.8.1.21 is shipping roughly on-par with 1.9.0.7 which does not contain this fix. Won't this give away the problem if we take this now?

Comment 44

[Boris Zbarsky [:bzbarsky]]

Er, I still need to do a 1.9.0.8 fix.  <sigh>.  I'll work on that.

Comment 45

[Boris Zbarsky [:bzbarsky]]

Attachment: 1.9.0 merge

Comment 46

[Daniel Veditz [:dveditz]]

Comment on attachment 365028 [details] [diff] [review]
1.9.0 merge

Approved for 1.9.0.8, a=dveditz for release-drivers

Comment 47

[Boris Zbarsky [:bzbarsky]]

Checked into CVS.

Comment 48

[Boris Zbarsky [:bzbarsky]]

Fun.  This sent CVS orange because the patch was wrong.  It replaced NS_IMPL_ELEMENT_CLONE_WITH_INIT with a clone method that doesn't call Init().  On 1.9.0 that leads to a crash running the mochitest for this bug.

On trunk, this got fixed as part of the checkin for bug 471551; see bug 471551 comment 20.  Going to push the equivalent change to branches ASAP so we go green and such and then post the patches for review.

Comment 49

[Boris Zbarsky [:bzbarsky]]

Attachment: 1.9.0 followup

Checked this in to 1.9.0 branch.

Comment 50

[Boris Zbarsky [:bzbarsky]]

Attachment: 1.9.1 followup

Pushed as http://hg.mozilla.org/releases/mozilla-1.9.1/rev/f195dc8f2d47

Comment 51

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

I can't get this to crash with 1.9.0.7 either on either Windows or OS X. Verified through the unit test (test_bug453736.html) passing on Tinderbox.

Comment 52

[Daniel Veditz [:dveditz]]

Comment on attachment 364373 [details] [diff] [review]
1.8 branch fix

Approved for 1.8.1.22, a=dveditz for release-drivers

Comment 53

[Boris Zbarsky [:bzbarsky]]

Checked in on 1.8.1 branch, with the followup to call Init() rolled into the patch.