784730 - "ASSERTION: wrapper already in new scope" with canvas, adoptNode

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Jesse Ruderman]

Attachment: testcase

With:

  user_pref("gfx.canvas.azure.enabled", false);

The testcase triggers:

###!!! ASSERTION: wrapper already in new scope!: '!newMap->Find(wrapper->GetIdentityObject())', file js/xpconnect/src/XPCWrappedNative.cpp, line 1651

Comment 1

[Jesse Ruderman]

Attachment: stack trace

Comment 2

[Andrew McCreight (out until 2026) [:mccr8]]

Azure is on by default everywhere in 18, so marking this unaffected for 18...

Comment 3

[Andrew McCreight (out until 2026) [:mccr8]]

I looked at this a little. Something seems funny with the interaction of getContext and adoptNode. When I add this alert, it alerts "false", but if I comment out |frameDoc.adoptNode(canvas)|, then it alerts "true".

    var frameDoc = document.getElementById("f").contentDocument;
    var canvas = document.createElement('canvas');
    var ctxA = canvas.getContext('2d');
    frameDoc.adoptNode(canvas);
    var ctxB = canvas.getContext('2d');
    alert(ctxA === ctxB);

I guess because of the adoptNode it is creating two XPCWNs for the nsCanvasRenderingContext2D, and then when we do the write, RescueOrphan attempts to reconcile things, and discovers the duplication?

Maybe this is expected behavior of the old DOM bindings we get with Azure disabled?

Comment 4

[Bobby Holley (:bholley)]

Ugh, so this is a pain. Continuing drama from bug 751995.

So, the canvas rendering context is parented to the canvas element:

http://mxr.mozilla.org/mozilla-central/source/content/canvas/public/nsICanvasRenderingContextInternal.h#53 (this is called by nsNewDOMBindingSH<T, BaseType>::PreCreate)

We first create a wrapper for the context in scope A. Then, we move the canvas to scope B. This orphans ctxA (in the sense that its parent object becomes a cross-compartment wrapper). Our solution for this over in bug 751995 was just leave to leave it that way until something like a document.open came along where this would be a problem, and then fix them on demand when we run into them. Unfortunately, we didn't think about the possibility of a new wrapper (in the other scope) having already been created by then.

Once we have two wrappers floating around, we're in kind of bad shape. At minimum, we've got two different steps of expandos floating around, and we have no clear algorithm on how to merge them.

If possible, I think the best solution is to implement nsWrapperCache for anything that has the potential to be orphaned. That way, XPCConvert::NativeInterface2JSObject will just find the existing wrapper on the nsISupports* and avoid calling into XPCWrappedNative::GetNewOrUsed, which won't find the existing wrapper because it's only looking in |Scope|.

This applies to all non-element objects parented to elements that the element doesn't fix up in CloneAndAdopt: .style, nodelists, canvas contexts, possibly other stuff. Some of them (like nodelists) already implement nsWrapperCache. Peter, do you know the full list here?

Comment 5

[Andrew McCreight (out until 2026) [:mccr8]]

I guess with the new DOM bindings for 2d contexts, which requires Azure being on, IIRC, we wrappercache them, which is why this example only fails with Azure off.

Comment 6

[Andrew McCreight (out until 2026) [:mccr8]]

Bobby: is there any kind of mitigation we can take that could be backported? Wrappercaching a bunch of classes doesn't sound simple. How bad of a security problem do you think this is? Losing expandos doesn't seem security sensitive, but I could imagine there being badness if you end up with multiple wrappers floating around.

Comment 7

[Bobby Holley (:bholley)]

(In reply to Andrew McCreight [:mccr8] from comment #6)
> Losing expandos doesn't seem security
> sensitive, but I could imagine there being badness if you end up with
> multiple wrappers floating around.

Yeah, that's what worries me. I'm sure there are ways to crash the browser in interesting ways when doing that.

Would wrapper caching canvas contexts really be all that bad? Too much to backport to beta at this point, but we could at least do it on aurora and such.

Also note that the regular CPG-esr10 story applies here.

Comment 8

[Andrew McCreight (out until 2026) [:mccr8]]

I think canvas contexts were converted to the new DOM bindings in 15 (bug 748266), but they are only used with Azure. I think Azure is only enabled everywhere as of 18, but I think it has been on Windows for longer?

Comment 9

[Nick Cameron [:nrc]]

(In reply to Andrew McCreight [:mccr8] from comment #8)
> I think canvas contexts were converted to the new DOM bindings in 15 (bug
> 748266), but they are only used with Azure. I think Azure is only enabled
> everywhere as of 18, but I think it has been on Windows for longer?

All correct. Azure canvas has also been enabled on mac for longer, I'm not sure exactly when it got turned on for mac/windows. The plan is for 19 to only have Azure canvas, 18 will have Thebes canvas, but it won't be used by default.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Azure on Windows was only available if you had direct2d, until recently, I thought.  So not on WinXP and not if your drivers were blacklisted...

Mac had Azure canvas before we landed the new DOM bindings.

Comment 11

[Andrew McCreight (out until 2026) [:mccr8]]

I don't really know much about this wrapper context stuff.  Maybe bholley has a better idea what to do here...

Comment 12

[Bobby Holley (:bholley)]

The best idea I've got is to wrapper-cache anything that can be orphaned: .style, nodelists, canvas contexts, what else?

Peter, can you brainstorm such a list?

Comment 13

[Daniel Veditz [:dveditz] back January 5, 2026]

since comment 4 says "continuing drama from bug 751995" I'm just going to guess this gets the same rating.

Comment 14

[David Bolter [:davidb] (NeedInfo me for attention)]

peterv, any updates re comment 12?

Comment 15

[David Bolter [:davidb] (NeedInfo me for attention)]

Updating tracking.

Comment 16

[Bobby Holley (:bholley)]

Maybe bz can answer instead.

Comment 17

[Peter Van der Beken [:peterv]]

(In reply to Bobby Holley (:bholley) from comment #12)
> The best idea I've got is to wrapper-cache anything that can be orphaned:
> .style, nodelists, canvas contexts, what else?
> 
> Peter, can you brainstorm such a list?

It's not easy, things are pretty spread out. Here's the list of DOM interfaces I have for now based on trunk code, but it's incomplete:

CanvasRenderingContext2D
DOMImplementation
DOMCSSDeclarationImpl
DOMSVGLengthList
DOMSVGNumberList
DOMSVGPathSegList
DOMSVGPointList
DOMSVGTransformList
HTMLPropertiesCollection
nsAnonymousContentList
nsClientRectList
nsComputedDOMStyle
nsCSSFontFaceStyleDecl
nsDOMCSSAttributeDeclaration
nsDOMFileList
nsDOMStringMap
nsDOMTokenList
nsFormControlList
nsHTMLOptionCollection
PropertyNodeList
TableRowsCollection
WebGLContext

Note that most of these are wrapper-cached on trunk (and converted to new bindings). We'd have to check for each whether they are actually affected on branches if we want to fix it there. Especially since we changed some of these to be parented to a node as part of wrapper-caching them, so even if they're not wrapper-cached on a branch that might not be a problem.

Comment 18

[Boris Zbarsky [:bzbarsky]]

The only sane way to get such a list is to grep for all interface names mentioned in any IDL inheriting from Node....

Comment 19

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

(In reply to Boris Zbarsky (:bz) from comment #18)
> The only sane way to get such a list is to grep for all interface names
> mentioned in any IDL inheriting from Node....

Are we going to do this? Are we moving towards an approach to a patch for this?

Comment 20

[Bobby Holley (:bholley)]

Andrew said he'd take this.

Comment 21

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

Andrew, any updates?

Comment 22

[Andrew McCreight (out until 2026) [:mccr8]]

nope

Comment 23

[Andrew McCreight (out until 2026) [:mccr8]]

I met with bholley, bz, peterv and mrbkap on this issue.  Apparently it is not a big deal to have two XPCWNs in different compartments.  Problems only arise when we attempt to create a wrapper in a compartment where there already is one, but that is easy to detect.  The new plan, then, is to make this assertion into a MOZ_CRASH(), thereby converting a security problem into a stability problem.  We can fix particular stability problems that show up by wrapper caching as needed, and otherwise just let this gradually happen as things are converted to the new DOM bindings.  Wrapper caching in general could have weird effects, so it may not be suitable for backporting.

Comment 24

[Andrew McCreight (out until 2026) [:mccr8]]

Attachment: crash in ReparentWrapperIfFound if wrapper already in new scope

https://tbpl.mozilla.org/?tree=Try&rev=28cdb558a0ad

Comment 25

[Andrew McCreight (out until 2026) [:mccr8]]

Comment on attachment 710625 [details] [diff] [review]
crash in ReparentWrapperIfFound if wrapper already in new scope

This had a clean Linux64 debug try run.

Comment 26

[Bobby Holley (:bholley)]

Comment on attachment 710625 [details] [diff] [review]
crash in ReparentWrapperIfFound if wrapper already in new scope

Add a comment to the MOZ_CRASH. r=bholley

Comment 27

[Andrew McCreight (out until 2026) [:mccr8]]

Comment on attachment 710625 [details] [diff] [review]
crash in ReparentWrapperIfFound if wrapper already in new scope

[Security approval request comment]
How easily could an exploit be constructed based on the patch?  Well, this is just converting an existing assertion to be fatal in release. So we're not exposing a new issue per se, but it does indicate that for some reason we started caring about this more, which could make somebody look at this more.  If somebody understands orphan rescue, it probably isn't too hard to recreate the test case (with something besides the azure stuff).

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?  Not particularly, the comments are just the same as the assert comment.

Which older supported branches are affected by this flaw? At this point, probably most or all of them.

If not all supported branches, which bug introduced the flaw?

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?  Just converting an old assertion, should be easy to update the patch.

How likely is this patch to cause regressions; how much testing does it need?  The basic problem here is that we're aren't fixing a problem, we're turning it from a security hazard into a controlled crash.  If this starts showing up in crash stats, then we'll have to land further patches that wrapper cache to fix the actual problem to fix the crashes, and those could be risky.  I'm not sure how common it is to adoptNode the same node repeatedly into multiple documents.

Comment 28

[Daniel Veditz [:dveditz] back January 5, 2026]

Comment on attachment 710625 [details] [diff] [review]
crash in ReparentWrapperIfFound if wrapper already in new scope

sec-appproval+

Comment 29

[Andrew McCreight (out until 2026) [:mccr8]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/35eda1ce617e

Comment 30

[Andrew McCreight (out until 2026) [:mccr8]]

Oops, I forgot to add the comment.  I'll land a followup in the next day or two.

Comment 31

[Andrew McCreight (out until 2026) [:mccr8]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/25766ae454f8

Comment 32

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/35eda1ce617e

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/25766ae454f8

Comment 34

[Lukas Blakk [:lsblakk] use ?needinfo]

Batch edit: Bugs still affected on b2g18 after 2/13 merge to v1.0.1 branch are affected on v1.0.1 branch.

Comment 35

[Andrew McCreight (out until 2026) [:mccr8]]

Comment on attachment 710625 [details] [diff] [review]
crash in ReparentWrapperIfFound if wrapper already in new scope

[Approval Request Comment]
Bug caused by (feature/regressing bug #): pretty old
User impact if declined: possible security problems
Testing completed: it has been on Nightly for a few weeks
Risk to taking this patch (and alternatives if risky): low.  any badness will show up in crash stats, and we can trivially back out the patch
String or UUID changes made by this patch: none

This has been on Nightly for a week or two, and I don't see any instances of crashes with "rescue" or "reparent" in the top 300 on crash stats for Nightly, so I think we're not hitting this in the wild, so this should be okay to uplift.

Comment 36

[Andrew McCreight (out until 2026) [:mccr8]]

Note to me or whoever lands this: make sure to land both patches.  The second one just adds a comment.

Comment 37

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 710625 [details] [diff] [review]
crash in ReparentWrapperIfFound if wrapper already in new scope

Approving for uplift, let's get this in now so we have time, if needed, to back out. For b2g18 this can be uplift to v1-train.

Comment 38

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/aeea02f63cb2
https://hg.mozilla.org/releases/mozilla-b2g18/rev/923fe8fd8a32
https://hg.mozilla.org/releases/mozilla-esr17/rev/f00cf94f3cb8

FWIW, I qfolded the two csets into one for the uplifts.

Comment 39

[Ioana (away)]

I've been trying to reproduce this issue on Firefox 20b1 (regular and debug builds), but it's not happening.

gfx.canvas.azure.enabled doesn't exist in about:config anymore, so I tried to reproduce this without it, and with it added manually.

Does anybody have any ideas about how else I could reproduce it?

Comment 40

[Andrew McCreight (out until 2026) [:mccr8]]

It is probably okay to not reproduce it.  My patch just turns an assertion into a runtime check.  I'm not sure why the particular test case doesn't reproduce.

Comment 41

[Huzaifa Sidhpurwala]

This crashes in 17.0.5 ESR, are you sure this issue is fixed there?

Comment 42

[Andrew McCreight (out until 2026) [:mccr8]]

(In reply to Huzaifa Sidhpurwala from comment #41)
> This crashes in 17.0.5 ESR, are you sure this issue is fixed there?

My patch here is kind of weird in that it takes potentially undefined behavior and turns it into a controlled crash.  If you look at the stack in the crash report in about:crashes, it should have MOZ_CRASH or something at the top.

As an aside, I looked at the list of crashes a little while ago, and didn't see anything that looked like the crash I introduced here, so fortunately this isn't something that people are getting in a normal browsing scenario.

Comment 43

[Alex Keybl [:akeybl]]

Since Firefox 20 is now released, v1.0.1 branches are still open, and this is considered low risk, we can uplift to v1.0.1. Marking as tef+ and flipping status-b2g18-v1.0.1 to affected.

Comment 44

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/4f3628f4ab79