Closed Bug 1121663 Opened 5 years ago Closed 5 years ago

SpecialPowers.loadChromeScript shouldn't wrap response messages

Categories

(Testing :: Mochitest, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla38

People

(Reporter: jld, Assigned: jld)

References

Details

Attachments

(1 file)

When a script loaded with SpecialPowers.loadChromeScript sends a message back to the content script, it's chrome-wrapped before passing it to the listener.  This means that it gets a Proxy which won't be accepted by, for example, a WebIDL binding expecting the actual object.  (Bug 1068838 ran into this trying to pass a DOM File.)

It looks as if nothing in the testsuite currently depends on  https://treeherder.mozilla.org/#/jobs?repo=try&revision=3b3ea5a0f39c — so I think it makes sense to remove it.  If any future test scripts need to wrap the returned object, they can always do that explicitly.
Attachment #8549154 - Flags: review?(ted)
Attachment #8549154 - Flags: feedback?(poirot.alex)
Comment on attachment 8549154 [details] [diff] [review]
bug1121663-sp-chromescript-unwrap-hg0.diff

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

Hum, from what I remember, if you were trying to access an attribute of an object that hasn't been wrapped,
you would get a security exception preventing you from accessing the attribute.
But given that it doesn't seem to break any test, that is either no longer the case or it never behaved like this and my memory is wrong.
Could you just triple check manually by trying to access and assert object properties values?
Attachment #8549154 - Flags: feedback?(poirot.alex) → feedback+
(In reply to Alexandre Poirot [:ochameau] from comment #2)
> Hum, from what I remember, if you were trying to access an attribute of an
> object that hasn't been wrapped,
> you would get a security exception preventing you from accessing the
> attribute.
> But given that it doesn't seem to break any test, that is either no longer
> the case or it never behaved like this and my memory is wrong.
> Could you just triple check manually by trying to access and assert object
> properties values?

My pending change for dom/base/test/test_bug403852.html has a chrome script that sends back an Object containing a number and a File; the content script can access the Object's properties without any wrapping or other special handling, which I think is the case you're asking about?
(In reply to Jed Davis [:jld] from comment #3)
> (In reply to Alexandre Poirot [:ochameau] from comment #2)
> > Hum, from what I remember, if you were trying to access an attribute of an
> > object that hasn't been wrapped,
> > you would get a security exception preventing you from accessing the
> > attribute.
> > But given that it doesn't seem to break any test, that is either no longer
> > the case or it never behaved like this and my memory is wrong.
> > Could you just triple check manually by trying to access and assert object
> > properties values?
> 
> My pending change for dom/base/test/test_bug403852.html has a chrome script
> that sends back an Object containing a number and a File; the content script
> can access the Object's properties without any wrapping or other special
> handling, which I think is the case you're asking about?

If I remember correctly it was only about native object (implementing a nsISomething interface).
Is File object having properties? Are you able to access them with this patch?
(In reply to Alexandre Poirot [:ochameau] from comment #4)
> If I remember correctly it was only about native object (implementing a nsISomething interface).
> Is File object having properties? Are you able to access them with this patch?

File is a WebIDL interface, not XPCOM, but the content script accesses the File's lastModifiedDate property, and that works.  Other native objects I'm not so sure about — wouldn't they need special structured clone handling (like while File has) to be sent over IPC like that?
(In reply to Jed Davis [:jld] from comment #5)
> (In reply to Alexandre Poirot [:ochameau] from comment #4)
> > If I remember correctly it was only about native object (implementing a nsISomething interface).
> > Is File object having properties? Are you able to access them with this patch?
> 
> File is a WebIDL interface, not XPCOM, but the content script accesses the
> File's lastModifiedDate property, and that works.

The reason this works is because the SpecialPowers compartment does forcePermissiveCOWs, so (if I understand the compartment/wrapper system correctly) anything created there has all properties exposed, recursively/transitively, when passed to content.  That didn't exist when loadChromeScript originally landed, so it would have needed the wrap() call at that time.
Depends on: 1038844
I didn't knew about forecePermissiveCOWS! f++ then ;)
Comment on attachment 8549154 [details] [diff] [review]
bug1121663-sp-chromescript-unwrap-hg0.diff

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

You sound like you know what you're doing, which is more than I can say. If you want an informed review I'd suggest tagging bholley, but if you're okay with rs=me then feel free. :)
Attachment #8549154 - Flags: review?(ted) → review+
https://hg.mozilla.org/mozilla-central/rev/c84bcdd07ff8
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in before you can comment on or make changes to this bug.