Last Comment Bug 453736 - [FIX]Crash under nsSVGElement::BindToTree with <svg:use>, <svg:script>
: [FIX]Crash under nsSVGElement::BindToTree with <svg:use>, <svg:script>
Status: VERIFIED FIXED
[sg:critical] see comment 27 for bran...
: assertion, crash, fixed-seamonkey1.1.16, fixed1.8.1.22, testcase, verified1.9.0.9, verified1.9.1
Product: Core
Classification: Components
Component: SVG (show other bugs)
: Trunk
: All All
: P1 critical (vote)
: mozilla1.9.2a1
Assigned To: Boris Zbarsky [:bz] (still a bit busy)
:
:
Mentors:
Depends on: 436965
Blocks: stirdom
  Show dependency treegraph
 
Reported: 2008-09-04 17:16 PDT by Jesse Ruderman
Modified: 2009-04-21 13:57 PDT (History)
15 users (show)
jonas: blocking1.9.1+
dveditz: blocking1.9.0.9+
dveditz: wanted1.9.0.x+
dveditz: blocking1.8.1.next+
dveditz: wanted1.8.1.x+
jruderman: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (crashes Firefox within a minute) (982 bytes, image/svg+xml)
2008-09-04 17:16 PDT, Jesse Ruderman
no flags Details
patch? (727 bytes, patch)
2008-09-05 12:50 PDT, Robert Longson
no flags Details | Diff | Splinter Review
Fix (2.18 KB, patch)
2008-10-29 23:28 PDT, Boris Zbarsky [:bz] (still a bit busy)
no flags Details | Diff | Splinter Review
Address review comments (10.36 KB, patch)
2008-12-12 08:34 PST, Boris Zbarsky [:bz] (still a bit busy)
no flags Details | Diff | Splinter Review
Comment fix that should land with the other patch. (857 bytes, patch)
2008-12-28 15:24 PST, Boris Zbarsky [:bz] (still a bit busy)
no flags Details | Diff | Splinter Review
Patch (13.77 KB, patch)
2009-01-13 19:04 PST, Boris Zbarsky [:bz] (still a bit busy)
jonas: review+
roc: review+
jonas: superreview+
Details | Diff | Splinter Review
1.9.1 version (14.00 KB, patch)
2009-01-14 05:36 PST, Boris Zbarsky [:bz] (still a bit busy)
no flags Details | Diff | Splinter Review
1.8 branch fix (1.47 KB, patch)
2009-02-26 13:04 PST, Boris Zbarsky [:bz] (still a bit busy)
jonas: review+
dveditz: approval1.8.1.next+
Details | Diff | Splinter Review
1.9.0 merge (15.03 KB, patch)
2009-03-02 14:31 PST, Boris Zbarsky [:bz] (still a bit busy)
dveditz: approval1.9.0.9+
Details | Diff | Splinter Review
1.9.0 followup (991 bytes, patch)
2009-03-18 08:09 PDT, Boris Zbarsky [:bz] (still a bit busy)
jonas: review+
jonas: superreview+
Details | Diff | Splinter Review
1.9.1 followup (779 bytes, patch)
2009-03-18 08:12 PDT, Boris Zbarsky [:bz] (still a bit busy)
jonas: review+
jonas: superreview+
Details | Diff | Splinter Review

Description Jesse Ruderman 2008-09-04 17:16:15 PDT
Created attachment 336942 [details]
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 2008-09-05 12:50:11 PDT
Created attachment 337106 [details] [diff] [review]
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 2008-09-06 00:51:00 PDT
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) 2008-10-04 19:24:03 PDT
Jess, does this affect the 1.9.0 branch?

Robert, any update here?
Comment 4 Robert Longson 2008-10-04 19:38:55 PDT
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 [:bz] (still a bit busy) 2008-10-29 23:28:05 PDT
Created attachment 345442 [details] [diff] [review]
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 2008-12-10 17:37:51 PST
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 [:bz] (still a bit busy) 2008-12-10 17:47:50 PST
> 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 2008-12-10 18:04:47 PST
(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 [:bz] (still a bit busy) 2008-12-10 19:14:26 PST
> 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 2008-12-10 19:16:47 PST
(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 2008-12-11 12:10:18 PST
Comment on attachment 345442 [details] [diff] [review]
Fix

Removing request while waiting for new patch
Comment 12 Boris Zbarsky [:bz] (still a bit busy) 2008-12-12 08:34:05 PST
Created attachment 352740 [details] [diff] [review]
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] 2008-12-17 16:05:07 PST
I don't crash nor see the comment 0 assertions in 1.9.0.5
Comment 14 Boris Zbarsky [:bz] (still a bit busy) 2008-12-18 15:28:15 PST
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) 2008-12-18 15:59:07 PST
Jesse, can you work up a modified testcase that affects 1.9.0.x?
Comment 16 Boris Zbarsky [:bz] (still a bit busy) 2008-12-18 16:02:03 PST
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 [:bz] (still a bit busy) 2008-12-28 15:24:50 PST
Created attachment 354634 [details] [diff] [review]
Comment fix that should land with the other patch.
Comment 18 Brandon Sterne (:bsterne) 2009-01-07 11:32:48 PST
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 [:bz] (still a bit busy) 2009-01-12 05:40:23 PST
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 2009-01-12 14:58:17 PST
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 2009-01-12 15:00:52 PST
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 [:bz] (still a bit busy) 2009-01-12 15:48:21 PST
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 2009-01-12 16:27:23 PST
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 [:bz] (still a bit busy) 2009-01-13 18:05:21 PST
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 [:bz] (still a bit busy) 2009-01-13 18:47:39 PST
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 [:bz] (still a bit busy) 2009-01-13 18:48:35 PST
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 [:bz] (still a bit busy) 2009-01-13 19:02:59 PST
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 [:bz] (still a bit busy) 2009-01-13 19:04:07 PST
Created attachment 356884 [details] [diff] [review]
Patch

I fixed the Run() thing while I was here.
Comment 29 Jonas Sicking (:sicking) No longer reading bugmail consistently 2009-01-13 19:16:40 PST
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 [:bz] (still a bit busy) 2009-01-13 19:22:45 PST
Comment on attachment 356884 [details] [diff] [review]
Patch

roc, want to double-check the view batch bit?
Comment 31 Boris Zbarsky [:bz] (still a bit busy) 2009-01-14 05:36:09 PST
Created attachment 356941 [details] [diff] [review]
1.9.1 version
Comment 33 Jesse Ruderman 2009-01-14 10:54:44 PST
in-testsuite+: patch contains a mochitest, original testcase is too slow to be a crashtest
Comment 34 Henrik Skupin (:whimboo) 2009-02-05 04:31:23 PST
Verified based on tinderbox status on trunk and 1.9.1 branch.
Comment 35 Samuel Sidler (old account; do not CC) 2009-02-23 07:08:28 PST
Boris: Can you write a 1.8 branch patch for this (per comment 27)?
Comment 36 Daniel Veditz [:dveditz] 2009-02-23 11:20:58 PST
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 [:bz] (still a bit busy) 2009-02-23 14:35:39 PST
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) 2009-02-23 14:56:13 PST
Boris: Does that mean we need to take bug 436965 on the 1.8.1 branch?
Comment 39 Boris Zbarsky [:bz] (still a bit busy) 2009-02-23 16:32:29 PST
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 [:bz] (still a bit busy) 2009-02-26 13:04:07 PST
Created attachment 364373 [details] [diff] [review]
1.8 branch fix
Comment 41 Jonas Sicking (:sicking) No longer reading bugmail consistently 2009-02-27 09:45:48 PST
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 [:bz] (still a bit busy) 2009-02-27 10:43:03 PST
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] 2009-02-27 13:52:56 PST
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 [:bz] (still a bit busy) 2009-02-27 16:59:36 PST
Er, I still need to do a 1.9.0.8 fix.  <sigh>.  I'll work on that.
Comment 45 Boris Zbarsky [:bz] (still a bit busy) 2009-03-02 14:31:52 PST
Created attachment 365028 [details] [diff] [review]
1.9.0 merge
Comment 46 Daniel Veditz [:dveditz] 2009-03-06 11:38:39 PST
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 [:bz] (still a bit busy) 2009-03-18 06:41:30 PDT
Checked into CVS.
Comment 48 Boris Zbarsky [:bz] (still a bit busy) 2009-03-18 08:06:09 PDT
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 [:bz] (still a bit busy) 2009-03-18 08:09:52 PDT
Created attachment 368023 [details] [diff] [review]
1.9.0 followup

Checked this in to 1.9.0 branch.
Comment 50 Boris Zbarsky [:bz] (still a bit busy) 2009-03-18 08:12:03 PDT
Created attachment 368024 [details] [diff] [review]
1.9.1 followup

Pushed as http://hg.mozilla.org/releases/mozilla-1.9.1/rev/f195dc8f2d47
Comment 51 Al Billings [:abillings] 2009-03-23 13:38:36 PDT
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] 2009-04-03 10:19:00 PDT
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 [:bz] (still a bit busy) 2009-04-08 10:11:21 PDT
Checked in on 1.8.1 branch, with the followup to call Init() rolled into the patch.

Note You need to log in before you can comment on or make changes to this bug.