781521 - Fix new __exposedProps__ culprits

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Bobby Holley (:bholley)]

In the last two months since we've been waiting to make __exposedProps__ mandatory (in bug 553102), a bunch of code has landed that goes orange when __exposedProps__ is required. Time to fix stuff again...

Comment 1

[Bobby Holley (:bholley)]

Attachment: Part 1 - Fix test_SpecialPowersExtension. v1

The constructor function lives in content, but it's being invoked from chrome, so the |this| object is actually privileged.

Comment 2

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 650934 [details] [diff] [review]
Part 1 - Fix test_SpecialPowersExtension. v1

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

I'm not a test peer.

Comment 3

[Bobby Holley (:bholley)]

Attachment: Part 2 - Fix Settings API. v1

Comment 4

[Bobby Holley (:bholley)]

Attachment: Part 3 - Fix BrowserElement API. v1

This one might still need some tweaking. There are some XXX comments in the code that I'm hoping justin can comment on.

Comment 5

[Bobby Holley (:bholley)]

Attachment: Part 4 - Fix Message Manager blob test. v1

Comment 6

[Bobby Holley (:bholley)]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #2)
> I'm not a test peer.

Yeah, but I was already flagging you for review on patch 4, since I think you're the one there who's most familiar with both the message manager and SpecialPowers wrapping. So I figured you could probably review this one-liner while you're at it.

Comment 7

[Justin Lebar (not reading bugmail)]

Comment on attachment 650938 [details] [diff] [review]
Part 3 - Fix BrowserElement API. v1

>diff --git a/dom/browser-element/BrowserElementParent.js b/dom/browser-element/BrowserElementParent.js
>+function exposeAll(obj) {
>+  // Filter for Objects and Arrays.
>+  if (typeof obj !== "object" || !obj)
>+    return;
>+
>+  // Recursively expose our children.
>+  Object.keys(obj).forEach(function(key) {
>+    exposeAll(obj[key]);
>+  });
>+
>+  // If we're not an Array, generate an __exposedProps__ object for ourselves.
>+  if (obj instanceof Array)
>+    return;
>+  var exposed = {};
>+  Object.keys(obj).forEach(function(key) {
>+    exposed[key] = 'rw';
>+  });
>+  obj.__exposedProps__ = exposed;
>+}

Oh, cool; I can make some things read-only?  I'll go back though and fix that up later.

>+function defineAndExpose(obj, name, value) {
>+  // XXXbholley - why do we need to waive Xray here? Are these objects sometimes
>+  // native and sometimes JS-implemented?

They sometimes come from the message manager.  I dunno if that answers your question.

>   _createEvent: function(evtName, detail, cancelable) {
>     // This will have to change if we ever want to send a CustomEvent with null
>     // detail.  For now, it's OK.
>     if (detail !== undefined && detail !== null) {
>+      if (typeof detail == "object")
>+        detail = JSON.parse(JSON.stringify(detail));

Is this to make sure that the object is "js-implemented" rather than native?
If so, do you still need to unwrap in defineAndExpose?

>+      exposeAll(detail);

>+++ b/dom/browser-element/mochitest/browserElement_PromptConfirm.js
>-        e.detail.returnValue = curPrompt.rv;
>+        // XXXbholley - This is probably wrong. What are the semantics of
>+        // returnValue? Is it supposed to be writable from content?
>+        SpecialPowers.wrap(e).detail.returnValue = curPrompt.rv;

Yes.  That's how content tells BrowserElementParent what the return value of the
prompt() should be.

I think you just need to set returnValue on the detail before dispatching the
event, and then we should be able to write it here, right?

>+++ b/dom/browser-element/mochitest/browserElement_Alert.js
>-    ok(true, msg.json);
>+    ok(true, 'Good!');//SpecialPowers.wrap(msg).json);
>   });
>   mm.addMessageListener('test-fail', function(msg) {
>     numPendingChildTests--;
>-    ok(false, msg.json);
>+    ok(false, 'Bad!');//SpecialPowers.wrap(msg).json);

I'd prefer the SpecialPowers.wrap(msg).json version, unless that doesn't work
for some reason.  Otherwise, these test logs will become much more difficult to
read ("Bad!" and "Good!" instead of a descriptive message).  We've had enough
problems with randomoranges in these tests that I'd like to keep the logs
useful, if we can.

This is good enough for r+, but we definitely need to figure out the returnValue business, because as written, this will break prompt().

Comment 8

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 650939 [details] [diff] [review]
Part 4 - Fix Message Manager blob test. v1

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

r=me, if you file a followup to see if this can be a chrome test instead.

Comment 9

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 650934 [details] [diff] [review]
Part 1 - Fix test_SpecialPowersExtension. v1

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

r=me, but a comment would be nice

Comment 10

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 650934 [details] [diff] [review]
Part 1 - Fix test_SpecialPowersExtension. v1

Consider these reviews officially delegated to Ms2ger.

Comment 11

[Bobby Holley (:bholley)]

Justin and I did an interactive discussion about IRC. Here are the results:

(In reply to Justin Lebar [:jlebar] from comment #7)

> >+function defineAndExpose(obj, name, value) {
> >+  // XXXbholley - why do we need to waive Xray here? Are these objects sometimes
> >+  // native and sometimes JS-implemented?
> 
> They sometimes come from the message manager.  I dunno if that answers your
> question.

We concluded that it wasn't necessary. I removed it.

> >+      if (typeof detail == "object")
> >+        detail = JSON.parse(JSON.stringify(detail));
> 
> Is this to make sure that the object is "js-implemented" rather than native?
> If so, do you still need to unwrap in defineAndExpose?

It was to deep clone the object to avoid exposing a potentially-held copy. Justin said it wasn't necessary, so I removed it.


> >+        // XXXbholley - This is probably wrong. What are the semantics of
> >+        // returnValue? Is it supposed to be writable from content?
> >+        SpecialPowers.wrap(e).detail.returnValue = curPrompt.rv;
> 
> Yes.  That's how content tells BrowserElementParent what the return value of
> the
> prompt() should be.
> 
> I think you just need to set returnValue on the detail before dispatching the
> event, and then we should be able to write it here, right?

I gave the calls to showModalPrompt for |alert| and |confirm| an initial returnValue of |undefined| so that exposeAll can pick up on it.
> >   mm.addMessageListener('test-fail', function(msg) {
> >     numPendingChildTests--;
> >-    ok(false, msg.json);
> >+    ok(false, 'Bad!');//SpecialPowers.wrap(msg).json);
> 
> I'd prefer the SpecialPowers.wrap(msg).json version, unless that doesn't work
> for some reason.  Otherwise, these test logs will become much more difficult
> to
> read ("Bad!" and "Good!" instead of a descriptive message).  We've had enough
> problems with randomoranges in these tests that I'd like to keep the logs
> useful, if we can.

Didn't mean to add that to the patch. Fixed.

Comment 12

[Bobby Holley (:bholley)]

Pushed to try:

https://tbpl.mozilla.org/?tree=Try&rev=204bad43c04e

Comment 13

[Bobby Holley (:bholley)]

Green on try. Pushed to m-i:

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/4f2aecfd6119
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/a0b22dc37b46
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/aed48f3c0fe1
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/98fe61542182

Comment 14

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/4f2aecfd6119
https://hg.mozilla.org/mozilla-central/rev/a0b22dc37b46
https://hg.mozilla.org/mozilla-central/rev/aed48f3c0fe1
https://hg.mozilla.org/mozilla-central/rev/98fe61542182