Last Comment Bug 781521 - Fix new __exposedProps__ culprits
: Fix new __exposedProps__ culprits
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: XPConnect (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: mozilla17
Assigned To: Bobby Holley (busy)
:
Mentors:
Depends on:
Blocks: 553102
  Show dependency treegraph
 
Reported: 2012-08-09 07:28 PDT by Bobby Holley (busy)
Modified: 2012-08-15 09:48 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Part 1 - Fix test_SpecialPowersExtension. v1 (1.24 KB, patch)
2012-08-10 10:27 PDT, Bobby Holley (busy)
Ms2ger: review+
Details | Diff | Review
Part 2 - Fix Settings API. v1 (2.16 KB, patch)
2012-08-10 10:30 PDT, Bobby Holley (busy)
anygregor: review+
Details | Diff | Review
Part 3 - Fix BrowserElement API. v1 (8.78 KB, patch)
2012-08-10 10:31 PDT, Bobby Holley (busy)
justin.lebar+bug: review+
Details | Diff | Review
Part 4 - Fix Message Manager blob test. v1 (1.27 KB, patch)
2012-08-10 10:31 PDT, Bobby Holley (busy)
Ms2ger: review+
Details | Diff | Review

Description Bobby Holley (busy) 2012-08-09 07:28:20 PDT
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 (busy) 2012-08-10 10:27:29 PDT
Created attachment 650934 [details] [diff] [review]
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 [:khuey] (khuey@mozilla.com) 2012-08-10 10:29:09 PDT
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 (busy) 2012-08-10 10:30:16 PDT
Created attachment 650937 [details] [diff] [review]
Part 2 - Fix Settings API. v1
Comment 4 Bobby Holley (busy) 2012-08-10 10:31:04 PDT
Created attachment 650938 [details] [diff] [review]
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 (busy) 2012-08-10 10:31:24 PDT
Created attachment 650939 [details] [diff] [review]
Part 4 - Fix Message Manager blob test. v1
Comment 6 Bobby Holley (busy) 2012-08-10 10:36:19 PDT
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) 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) 2012-08-10 10:50:50 PDT
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 2012-08-14 09:41:14 PDT
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 2012-08-14 09:43:27 PDT
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 [:khuey] (khuey@mozilla.com) 2012-08-14 09:52:24 PDT
Comment on attachment 650934 [details] [diff] [review]
Part 1 - Fix test_SpecialPowersExtension. v1

Consider these reviews officially delegated to Ms2ger.
Comment 11 Bobby Holley (busy) 2012-08-14 10:29:10 PDT
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 (busy) 2012-08-14 11:18:26 PDT
Pushed to try:

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

Note You need to log in before you can comment on or make changes to this bug.