Closed Bug 1389585 Opened 3 years ago Closed 2 years ago

Does XPCJSID need DOM_OBJECT classinfo?

Categories

(Core :: XPConnect, enhancement, P3)

53 Branch
enhancement

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file)

It's not clear to me that it really does...
Priority: -- → P3
For what it's worth, https://treeherder.mozilla.org/#/jobs?repo=try&revision=814bafb1407313538999bac929b86c464d2cceb4 is quite orange because of SpecialPowers bits that apparently rely on this.  I haven't had a chance to think about how those can be changed yet.
Blocks: 1425511
Blocks: 1207321
Depends on: 1448398
Depends on: 1448397
Blocks: 1448414
These are not supposed to be exposed to content.

MozReview-Commit-ID: 3odHUn4ZlG
Attachment #8961994 - Flags: review?(peterv)
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Comment on attachment 8961994 [details] [diff] [review]
Stop giving XPCJSID DOM_OBJECT classinfo

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

r=me assuming tests pass. I have a nagging suspicion that there might be some dark corners of tests where we still rely on passing IIDs to the content side of mochitests...

::: dom/indexedDB/test/helpers.js
@@ +17,5 @@
>  // is whether the property is read-only or not.
>  var c = Object.getOwnPropertyDescriptor(this, "Components");
>  if ((!c.value || c.writable) && typeof SpecialPowers === "object") {
>    // eslint-disable-next-line no-native-reassign
> +  Components = SpecialPowers.wrap(SpecialPowers.Components);

This shouldn't be necessary after bug 1448736. It won't even work with the DOM_OBJECT flag removed from Components, since we won't even be able to create an unprivileged wrapper to return from SpecialPowers.
Attachment #8961994 - Flags: review?(peterv) → review+
> This shouldn't be necessary after bug 1448736.

It's necessary.

SpecialPowers.Components is in fact a raw Components object, in the SpecialPowers compartment.

By the time we're looking at it from this compartment, we need to get a wrapper.  Since Components has classinfo with PreCreate (pointing back to its XPCWrappedNativeScope's global), it does _not_ get a new reflector in our compartment.  So CanCreateWrapper is not invoked and the DOM_OBJECT bits don't matter.

What does matter is that we're dealing with SpecialPowers.  And specialpowersAPI.js calls Cu.forcePermissiveCOWs.  That means that when we land in WrapperFactory::Rewrap originCompartmentPrivate->forcePermissiveCOWs is true and we create a js::CrossCompartmentWrapper for the Components.

So we have the Components and we can get things from it.  But if we do Components.utils, then we will get an exception, because the utils object does _not_ have a PreCreate hook, and we will in fact try to create a new XPCWrappedNative, which will fail the CanCreateWrapper check once bug 1389581 lands.  And before that lands, doing Components.interfaces.nsIFoo will similarly fail CanCreateWrapper for the IID object, due to the change in this bug, which is why I'm making this change here.

I suspect removing the forcePermissiveCOWs thing would mean we couldn't self-host the SpecialPowersHandler, by the way, which is why it's there.
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/aa72b81f6c65
Stop giving XPCJSID DOM_OBJECT classinfo.  r=kmag
https://hg.mozilla.org/mozilla-central/rev/aa72b81f6c65
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.