Closed Bug 1430164 Opened 2 years ago Closed 2 years ago

Node !== this.Node !== window.Node in content scripts

Categories

(WebExtensions :: General, defect)

57 Branch
defect
Not set

Tracking

(firefox59 verified)

VERIFIED FIXED
mozilla59
Tracking Status
firefox59 --- verified

People

(Reporter: Tomislav, Assigned: bzbarsky)

Details

Attachments

(4 files, 2 obsolete files)

Attached file xrays.zip
This is confusing, because all of these (in global scope):

    Node
    this.Node
    window.Node

should be Xray wrappers to the same DOM constructor, and by our CCW rules, they should all be the same wrapper.  In fact, comparing `.wrappedJSObject`s shows them as equal.

I'm doing mozregression now (with the attached extension on example.com), and this seems to be the behavior for at least a year.
Hey Boris, you can load the attached extension temporarily and open http://example.com/
Flags: needinfo?(bzbarsky)
So in this case window.Node is an Xray, as expected.

Node and this.Node are both sandboxCallableProxyHandler proxies around that Xray.  Because WrapCallable just creates a new thing every time, Node !== this.Node.  For that matter, Node !== Node.  This is happening because when we get stuff off the sandbox global's prototype the fix for bug 

When you do .wrappedJSObject the sandboxCallableProxyHandler forwards the lookup to the Xray, then calls the resulting xpc::wrappedJSObject_getter with itself.  This actually fatally asserts in a debug build:

  Assertion failure: js::IsCrossCompartmentWrapper(wrapper), at /home/bzbarsky/mozilla/vanilla/mozilla/js/xpconnect/wrappers/WrapperFactory.cpp:117

because "wrapper" is the sandboxCallableProxyHandler thing.  But in an opt build we press on, unwrap things (which unwraps the sandboxCallableProxyHandler, since that's a js::Wrapper), and create a waiver.  Which is why in an opt build .wrappedJSObject matches for all three things.  I filed bug 1430169 on sorting out what should happen here.

Back to the main problem... The idea of sandboxCallableProxyHandler is to lie to the callee function about what `this` it's invoked with, making it look like the sandboxProto instead of the sandbox global.  Since generally speaking DOM constructors don't care, we could simply skip creating sandboxCallableProxyHandlers for them.  You'd still have the problem that alert !== window.alert != this.alert, though, and that one really does care about the "this" it's called with so needs this hackery.
Flags: needinfo?(bzbarsky) → needinfo?(bobbyholley)
> This is happening because when we get stuff off the sandbox global's prototype the fix for bug 

... the fix for bug 726949 (and bug 771429) kicks in.
This fixes DOM constructor identity in web extension content scripts to work as expected.

MozReview-Commit-ID: Ab4sFWiMU6c
Attachment #8942263 - Flags: review?(bobbyholley)
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Comment on attachment 8942263 [details] [diff] [review]
Stop doing weird sandbox-callable-wrapping for DOM constructors

Review of attachment 8942263 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with a test added.

::: js/xpconnect/src/Sandbox.cpp
@@ +771,5 @@
> +    }
> +
> +    const js::Class* clasp = js::GetObjectClass(obj);
> +    return dom::IsDOMIfaceAndProtoClass(clasp) &&
> +        dom::DOMIfaceAndProtoJSClass::FromJSClass(clasp)->mType == dom::eInterface;

Nit: alignment.

Also, I'm a bit surprised we don't have a utility method for this already. If we really don't, can you put this in BindingUtils or xpcpublic or somesuch?
Attachment #8942263 - Flags: review?(bobbyholley) → review+
This fixes DOM constructor identity in web extension content scripts to work as expected.

MozReview-Commit-ID: Ab4sFWiMU6c
Attachment #8942273 - Flags: review?(bobbyholley)
Attachment #8942263 - Attachment is obsolete: true
> Also, I'm a bit surprised we don't have a utility method for this already.

We don't.

I guess I can put it in BindingUtils as long as you don't mind it unwrapping and checking the underlying object, which is sort of slightly specific to this one consumer...

(I tried having a dom::IsDOMConstructor that did all but the unwrapping bit and a static IsDOMConstructor that calls it, but the IsDOMConstructor call is ambiguous because someone in that dir does "using namespace mozilla::dom" and we do unified builds...)
Comment on attachment 8942273 [details] [diff] [review]
Stop doing weird sandbox-callable-wrapping for DOM constructors

Review of attachment 8942273 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the function factored out.
Attachment #8942273 - Flags: review?(bobbyholley) → review+
Attachment #8942273 - Attachment is obsolete: true
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8be5c5375a90
Stop doing weird sandbox-callable-wrapping for DOM constructors. r=bholley
https://hg.mozilla.org/mozilla-central/rev/8be5c5375a90
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Flags: needinfo?(bobbyholley)
Attached image node.gif
Tested and reproduced this issue on 59.0a1 (20180111100722) Windows 10 64Bit.
I am still able to reproduce the issue on 59.0a1 (20180118012136). Is this the desired behavior or is a different extension required?
Flags: needinfo?(bzbarsky)
What issue do you think you reproduced?   The attached gif moves too fast for me to be able to tell what it's trying to show me...
Flags: needinfo?(bzbarsky) → needinfo?(marius.santa)
I thought the desired behavior is for the pop not to be displayed. 
Please provide a scenario with expected results or a different extension to test.
Flags: needinfo?(marius.santa) → needinfo?(bzbarsky)
The desired behavior is for the popup to show the correct values.  In particular, it needs to show 'true' for A === B, A === C, and B === C.
Flags: needinfo?(bzbarsky)
Attached image node working.gif
I was able to reproduce the issue on Windows 10 64Bit Firefox 59.0a1 (20180112220334).
Tested and verified the issue in I was able to reproduce the issue on Windows 10 64Bit Firefox 59.0a1 (20180122100120).
Status: RESOLVED → VERIFIED
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.