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

VERIFIED FIXED in mozilla1.9.2a1

Status

()

Core
SVG
P1
critical
VERIFIED FIXED
9 years ago
8 years ago

People

(Reporter: Jesse Ruderman, Assigned: bz)

Tracking

(Blocks: 1 bug, 7 keywords)

Trunk
mozilla1.9.2a1
assertion, crash, fixed-seamonkey1.1.16, fixed1.8.1.22, testcase, verified1.9.0.9, verified1.9.1
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.1 +
blocking1.9.0.9 +
wanted1.9.0.x +
blocking1.8.1.next +
wanted1.8.1.x +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical] see comment 27 for branches)

Attachments

(7 attachments, 4 obsolete attachments)

982 bytes, image/svg+xml
Details
13.77 KB, patch
sicking
: review+
roc
: review+
sicking
: superreview+
Details | Diff | Splinter Review
14.00 KB, patch
Details | Diff | Splinter Review
1.47 KB, patch
sicking
: review+
Details | Diff | Splinter Review
15.03 KB, patch
Details | Diff | Splinter Review
991 bytes, patch
sicking
: review+
sicking
: superreview+
Details | Diff | Splinter Review
779 bytes, patch
sicking
: review+
sicking
: superreview+
Details | Diff | Splinter Review
(Reporter)

Description

9 years ago
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.
(Reporter)

Updated

9 years ago
Whiteboard: [sg:critical]
Flags: blocking1.9.1+

Comment 1

9 years ago
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?
(Reporter)

Comment 2

9 years ago
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.
Flags: wanted1.9.0.x?
Jess, does this affect the 1.9.0 branch?

Robert, any update here?

Comment 4

9 years ago
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.
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.
Assignee: nobody → bzbarsky
Attachment #337106 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #345442 - Flags: superreview?(jonas)
Attachment #345442 - Flags: review?(jonas)
Summary: Crash under nsSVGElement::BindToTree with <svg:use>, <svg:script> → [FIX]Crash under nsSVGElement::BindToTree with <svg:use>, <svg:script>
Whiteboard: [sg:critical] → [sg:critical][needs review]
Priority: -- → P2
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?)
> 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.
(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.
> 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>?
(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 on attachment 345442 [details] [diff] [review]
Fix

Removing request while waiting for new patch
Attachment #345442 - Flags: superreview?(jonas)
Attachment #345442 - Flags: review?(jonas)
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?
Attachment #345442 - Attachment is obsolete: true
Attachment #352740 - Flags: superreview?(jonas)
Attachment #352740 - Flags: review?(jonas)
I don't crash nor see the comment 0 assertions in 1.9.0.5
Flags: wanted1.9.0.x? → wanted1.9.0.x-
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.
Jesse, can you work up a modified testcase that affects 1.9.0.x?
Flags: wanted1.9.0.x- → wanted1.9.0.x?
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.
Created attachment 354634 [details] [diff] [review]
Comment fix that should land with the other patch.
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!
With the expanded changes Jonas asked for, this needs to go into b3, in my opinion.
Priority: P2 → P1
Attachment #352740 - Flags: superreview?(jonas)
Attachment #352740 - Flags: superreview+
Attachment #352740 - Flags: review?(jonas)
Attachment #352740 - Flags: review+
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.
Attachment #352740 - Flags: superreview?(jonas)
Attachment #352740 - Flags: superreview+
Attachment #352740 - Flags: review?(jonas)
Attachment #352740 - Flags: review+
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
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.
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
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?
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.
Depends on: 436965
Er, I was wrong.  There was a script blocker assert in ProcessPendingRestyles, but not in ProcessRestyledFrames, which had some non-script-blocking entry points.
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.
Created attachment 356884 [details] [diff] [review]
Patch

I fixed the Run() thing while I was here.
Attachment #352740 - Attachment is obsolete: true
Attachment #354634 - Attachment is obsolete: true
Attachment #356884 - Flags: superreview?(jonas)
Attachment #356884 - Flags: review?(jonas)
Attachment #352740 - Flags: superreview?(jonas)
Attachment #352740 - Flags: review?(jonas)
Attachment #356884 - Flags: superreview?(jonas)
Attachment #356884 - Flags: superreview+
Attachment #356884 - Flags: review?(jonas)
Attachment #356884 - Flags: review+
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 on attachment 356884 [details] [diff] [review]
Patch

roc, want to double-check the view batch bit?
Attachment #356884 - Flags: review?(roc)
Attachment #356884 - Flags: review?(roc) → review+
Created attachment 356941 [details] [diff] [review]
1.9.1 version
Pushed http://hg.mozilla.org/mozilla-central/rev/b11d1f574c32 and http://hg.mozilla.org/releases/mozilla-1.9.1/rev/42e0d4b2c804
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Keywords: fixed1.9.1
Resolution: --- → FIXED
(Reporter)

Comment 33

9 years ago
in-testsuite+: patch contains a mochitest, original testcase is too slow to be a crashtest
Flags: in-testsuite+
Verified based on tinderbox status on trunk and 1.9.1 branch.
Status: RESOLVED → VERIFIED
Keywords: fixed1.9.1 → verified1.9.1
OS: Mac OS X → All
Hardware: x86 → All
Whiteboard: [sg:critical][needs review] → [sg:critical]
Target Milestone: --- → mozilla1.9.2a1
Flags: wanted1.9.0.x?
Flags: wanted1.9.0.x+
Flags: wanted1.8.1.x+
Flags: blocking1.9.0.8+
Flags: blocking1.8.1.next?
Whiteboard: [sg:critical] → [sg:critical] see comment 27 for branches
Flags: blocking1.8.1.next? → blocking1.8.1.next+
Boris: Can you write a 1.8 branch patch for this (per comment 27)?
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).
Flags: blocking1.8.1.next+ → blocking1.8.1.next?
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.
Boris: Does that mean we need to take bug 436965 on the 1.8.1 branch?
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.
Created attachment 364373 [details] [diff] [review]
1.8 branch fix
Attachment #364373 - Flags: review?(jonas)
Attachment #364373 - Flags: approval1.9.0.8?
Attachment #364373 - Flags: approval1.9.0.8? → approval1.8.1.next?
Attachment #364373 - Flags: review?(jonas) → review+
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?
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 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?
Er, I still need to do a 1.9.0.8 fix.  <sigh>.  I'll work on that.
Created attachment 365028 [details] [diff] [review]
1.9.0 merge
Attachment #365028 - Flags: approval1.9.0.8?
Comment on attachment 365028 [details] [diff] [review]
1.9.0 merge

Approved for 1.9.0.8, a=dveditz for release-drivers
Attachment #365028 - Flags: approval1.9.0.8? → approval1.9.0.8+
Checked into CVS.
Keywords: fixed1.9.0.8
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.
Created attachment 368023 [details] [diff] [review]
1.9.0 followup

Checked this in to 1.9.0 branch.
Attachment #368023 - Flags: superreview?(jonas)
Attachment #368023 - Flags: review?(jonas)
Created attachment 368024 [details] [diff] [review]
1.9.1 followup

Pushed as http://hg.mozilla.org/releases/mozilla-1.9.1/rev/f195dc8f2d47
Attachment #368024 - Flags: superreview?(jonas)
Attachment #368024 - Flags: review?(jonas)
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.
Keywords: fixed1.9.0.8 → verified1.9.0.8
Flags: blocking1.8.1.next? → blocking1.8.1.next+
Comment on attachment 364373 [details] [diff] [review]
1.8 branch fix

Approved for 1.8.1.22, a=dveditz for release-drivers
Attachment #364373 - Flags: approval1.8.1.next? → approval1.8.1.next+
Attachment #368023 - Flags: superreview?(jonas)
Attachment #368023 - Flags: superreview+
Attachment #368023 - Flags: review?(jonas)
Attachment #368023 - Flags: review+
Attachment #368024 - Flags: superreview?(jonas)
Attachment #368024 - Flags: superreview+
Attachment #368024 - Flags: review?(jonas)
Attachment #368024 - Flags: review+

Updated

9 years ago
Keywords: fixed-seamonkey1.1.16
Checked in on 1.8.1 branch, with the followup to call Init() rolled into the patch.
Keywords: fixed1.8.1.22
Group: core-security
You need to log in before you can comment on or make changes to this bug.