732870 - "ASSERTION: Weird scope returned" with document.write twice, dataset, adoptNode

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Jesse Ruderman]

Attachment: testcase

###!!! ASSERTION: Weird scope returned: 'betterScope == newScope', file js/xpconnect/src/nsXPConnect.cpp, line 1771

(Previous episodes: bug 716383, bug 731471.)

Comment 1

[Jesse Ruderman]

Attachment: stack trace

Comment 2

[Andrew McCreight [:mccr8]]

Still affects 16.

Comment 3

[Jesse Ruderman]

Now it also triggers:

Assertion failure: compartment mismatched, at js/src/jscntxtinlines.h:237

Comment 4

[Jesse Ruderman]

Attachment: testcase 2

Comment 5

[Jesse Ruderman]

Attachment: stack traces 2

Comment 6

[Bobby Holley (:bholley)]

I'll try to take a look at this.

Comment 7

[Bobby Holley (:bholley)]

So the issue here is that nsDOMStringMapSH::PreCreate uses its ownerDocument to find the parent, but requests to be parented directly to the window. This breaks our orphan detection over in bug 751995, because the wrapper isn't orphaned: It's still in the scope of the window, which is its parent. But since PreCreate finds the window via the document (which has moved), calling PreCreate again will give a different answer.

I don't see any obvious reason why nsDOMStringMap wrappers can't just be parented to the document, like Node. I've got a patch to do this that I'll upload and pushed to try.

Comment 8

[Bobby Holley (:bholley)]

Attachment: Have nsDOMStringMapSH::PreCreate use the document as the parent, rather than the window. v1

Comment 9

[Bobby Holley (:bholley)]

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=dd59a366d436

Comment 10

[Bobby Holley (:bholley)]

Attachment: Have nsDOMStringMapSH::PreCreate use the element as its parent, rather than the window. v2

Arg, this wasn't quite right. The crashtest is non-deterministic (since it depends the order in which wrappers are reparented), so I didn't realize that this wasn't fixed in all cases.

The issue here was that nsDOMStringMap::PreCreate actually finds its document via its associated element. And in this case, the element gets adopted into another document. Our orphan fixup code is supposed to detect when the parent moves, but in this case the parent (the document) didn't move.

So I think we should actually use the element as the parent. Attaching a patch that does that.

Comment 11

[Bobby Holley (:bholley)]

Repushed to try: https://tbpl.mozilla.org/?tree=Try&rev=69c4caecc2de

Comment 12

[Jesse Ruderman]

Can the non-deterministic crashtest be turned into two deterministic crashtests?

Comment 13

[Bobby Holley (:bholley)]

(In reply to Jesse Ruderman from comment #12)
> Can the non-deterministic crashtest be turned into two deterministic
> crashtests?

I played around with it a bit just now, but I don't think there's a good way.

More to the point, it's only non-deterministic if we parent it to the document (my v1 patch, which we're not doing). It's deterministic against trunk.

Comment 14

[Bobby Holley (:bholley)]

Pushed to m-i: http://hg.mozilla.org/integration/mozilla-inbound/rev/eca1167488ca

Comment 15

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/b37799afa959

Comment 16

[Daniel Veditz [:dveditz]]

The code being patched looks like it is in ESR, but it also might be fallout from cpg which wouldn't affect ESR.

Does it?

Comment 17

[Bobby Holley (:bholley)]

This is basically just patching an edge case in the fix from bug 751995. Same issue.

Comment 18

[Daniel Veditz [:dveditz]]

Bug 751995 is fixing a CPG regression, so it'd be nice to fix this in the release we landed CPG. Please request Aurora approval to land this for Firefox 15.

Comment 19

[Bobby Holley (:bholley)]

Comment on attachment 634343 [details] [diff] [review]
Have nsDOMStringMapSH::PreCreate use the element as its parent, rather than the window. v2

Per comment 18.

Comment 20

[Bobby Holley (:bholley)]

Bug caused by (feature/regressing bug #): CPG
User impact if declined: Potential security exploits.
Testing completed (on m-c, etc.): Baked on m-c for quite some time.
Risk to taking this patch (and alternatives if risky): not risky.
String or UUID changes made by this patch: None

Comment 21

[Alex Keybl [:akeybl]]

Comment on attachment 634343 [details] [diff] [review]
Have nsDOMStringMapSH::PreCreate use the element as its parent, rather than the window. v2

[Triage Comment]
Low risk fix for a new sg:high regression in FF15. Approved for Aurora 15.

Comment 22

[Bobby Holley (:bholley)]

Landed to m-a:
http://hg.mozilla.org/releases/mozilla-aurora/rev/f123feca7157