Migrate SpecialPowers' mock objects 'exposeAll' function away from exposedProps

NEW
Unassigned

Status

4 years ago
4 years ago

People

(Reporter: Gijs, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 obsolete attachment)

(Reporter)

Comment 1

4 years ago
Unfortunately, this is harder than I thought, because all of these have an |init| function where a window gets passed in, and that breaks because we can't do a structured clone of a window. AIUI that's intentional, but I'm not 100% sure how best to rearchitecture here so that problem goes away... Bobby, do you have ideas here, or should I deprio this and work on some non-test issues first?
Flags: needinfo?(bobbyholley)
Depends on: 1027131
When bug 1027131 lands, you'll be to pass { wrapReflectors: true } to Cu.cloneInto, and DOM objects will be cross-compartment wrapped.
Flags: needinfo?(bobbyholley)
(Reporter)

Comment 3

4 years ago
Created attachment 8448847 [details] [diff] [review]
remove exposeAll __exposedProps__ usage from mocks,
Attachment #8448847 - Flags: feedback?(bobbyholley)
(Reporter)

Comment 4

4 years ago
Oy. bzexport apparently lost my entire comment. Delightful. I wrote:

Apologies for the delay here; this now at least calls the function. The new problem is that when using cloneInto with cloneFunctions, we don't keep the |this| references in any way, and therefore when running a test, |this| in Mock*.init is the module's global (ie backstagepass), which means their |this.reset()| calls fail (and assigning |this.window = window| also isn't what the code expects). We could fix this by updating the consumers to use more strongly bound functions (not very pretty, but it'd work), but really, I think we should make cloneInto deal with this if possible. Bobby, is this a hard thing to add?


To test this, just apply the patch, rebuild testing/specialpowers, and run:

./mach mochitest-browser browser/base/content/test/general/browser_save_link-perwindowpb.js  --jsdebugger

Set a JS breakpoint in MockFilePicker.init on the this.reset() line.
Comment on attachment 8448847 [details] [diff] [review]
remove exposeAll __exposedProps__ usage from mocks,

So looking at this, I don't think this is the right approach (sorry for not noticing that earlier).

These Mock objects are JS-implemented XPCOM components. When we createInstance them, we end up with a non-WebIDL XPCOM object. And then the cloneInto call with { wrapReflectors: true } causes us to wrap that XPCOM object into content, which ends up creating a new XPCWrappedNative for it in the content scope. This is something we're trying very hard to eliminate (see bug 981845), so we shouldn't add any more of it here.

So I think we should just leave all the SpecialPowers stuff be for now, and then come up with some kind of one-off hack for it when it's the only thing left.
Attachment #8448847 - Flags: feedback?(bobbyholley) → feedback-
(Reporter)

Comment 6

4 years ago
OK!
Assignee: gijskruitbosch+bugs → nobody
(Reporter)

Updated

4 years ago
Attachment #8448847 - Attachment is obsolete: true
(Reporter)

Updated

4 years ago
Summary: Migrate SpecialPowers' mock objects 'exposeAll' function to Cu.cloneInto with {cloneFunctions: true} → Migrate SpecialPowers' mock objects 'exposeAll' function away from exposedProps
You need to log in before you can comment on or make changes to this bug.