821633 - Alarm API - .getAll() need to wrap the objects respecting to the content window.

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Comment 1

[Gene Lian (I already quit Mozilla)]

Please see [1]. Currently, .getAll() returns [{},{},{}] (3 alarms are set for example). We need to re-wrap the returned objects subjecting to the content window so that they can be exposed to the web contents.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=821138#c6

Comment 2

[Gene Lian (I already quit Mozilla)]

Attachment: Part 1, ObjectWrapper.jsm

Hi Fabrice,

When I tried to use .wrap(), I found the Date object cannot be properly exposed to web contents. For example, when the alarm fires, the alarm system message carries a Date object. The system message handler on the web content side would see:

{"id":6,"date":{},"respectTimezone":"ignoreTimezone","data":{"id":6}}

With this patch, it will correctly see:

{"id":6,"date":"2012-12-14T12:03:00.000Z","respectTimezone":"ignoreTimezone","data":{"id":6}}

What do you think?

Comment 3

[Gene Lian (I already quit Mozilla)]

Attachment: Part 2, Alarm API

Hi Vivien,

1. We need to expose the objects returned by .getAll(). It's backward-compatible to do this, because the apps only use .length to know whether any alarm has been added.

2. Also, I think we don't need to expose too many internal attributes to contents in the system message. This is also backward-compatible because all the apps only use |data| so far.

3. Both #1 and #2 now return:
  
  "id"
  "date"
  "respectTimezone"
  "data"

which are exactly the parameters and returned ID of .add() as below:

  DOMRequest add(in jsval date, in DOMString respectTimezone, [optional] in jsval data);

Could you please take this review? Thanks a lot!

Comment 4

[Gene Lian (I already quit Mozilla)]

(In reply to Gene Lian [:gene] from comment #2)
> Created attachment 692269 [details] [diff] [review]
> Part 1, ObjectWrapper.jsm

Hi Fabrice,

Please let me know if you prefer using .getMonth() at [1].

[1] http://stackoverflow.com/questions/643782/how-to-know-if-an-object-is-a-date-or-not-with-javascript

Comment 5

[Gene Lian (I already quit Mozilla)]

Hi David,

If you have a chance, could you please take a look at comment #2 about why we need to treat the Date object as "primitive" instead of "object" when wrapping it? Thanks!

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 692269 [details] [diff] [review]
Part 1, ObjectWrapper.jsm

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

Hmm... wait.. we need to do |new aCtxt.Date(aObject)|.

In other words, you should return "date" here and add a new branch to the wrap function.

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

And while you are here. Can you change

return new aCtxt.Blob([aObject]);

to

return new aCtxt.Blob([aObject], { type: aObject.type });

Comment 8

[David Flanagan [:djf]]

(In reply to Gene Lian [:gene] from comment #5)
> Hi David,
> 
> If you have a chance, could you please take a look at comment #2 about why
> we need to treat the Date object as "primitive" instead of "object" when
> wrapping it? Thanks!

I think Jonas answered this question. I'd prefer to not pass the date object to the Date constructor (because I don't understand why it works), so I'd modify Jonas' suggestion to this:

  new aCtxt.Date(aObject.getTime())

Comment 9

[David Flanagan [:djf]]

Also, while we're fixing up ObjectWrapper.jsm, shouldn't we do something sensible with functions? Right now they're treated as primitives and passed through unmodified.  Is that even possible? Is it desireable or is it a security hole?

I propose that functions just don't get wrapped. If you pass a function to wrap() it returns null.  And if you pass an object with methods to wrap(), then those function-valued properties are just omitted from the wrapped object.

I'm assuming that wrapped functions don't work in the receiver so there can't be any code relying on the current behavior, but I suppose that might not be right.

Comment 10

[[:fabrice] Fabrice Desré]

We never meant to wrap functions so I'm ok with them being nullified currently. Of course we can revisit that if we have valid use cases.

Comment 11

[David Flanagan [:djf]]

Fabrice: are they being nullified? I thought they were being treated as primitives and so passed through unmodified.

Comment 12

[Gene Lian (I already quit Mozilla)]

(In reply to Jonas Sicking (:sicking) from comment #6)
> Comment on attachment 692269 [details] [diff] [review]
> Part 1, ObjectWrapper.jsm
> 
> Review of attachment 692269 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Hmm... wait.. we need to do |new aCtxt.Date(aObject)|.

After some experiments, that's not lucky we can do this like new |new aCtxt.Blob(...)| or |new aCtxt.File(...)|. The log returns me:

  "aCtxt.Date is not a constructor"

The |aCtxt| is passed by the |aWindow| of |nsIDOMGlobalPropertyInitializer.init(aWindow)| in the AlarmsManager.js [1]. Using |this._window| inherited from |DOMRequestIpcHelper| doesn't work either (which seems the same window). May I have your suggestions or workarounds regarding this issue please?

[1] http://mxr.mozilla.org/mozilla-central/source/dom/alarm/AlarmsManager.js#137

Comment 13

[Gene Lian (I already quit Mozilla)]

(In reply to David Flanagan [:djf] from comment #8)
> I think Jonas answered this question. I'd prefer to not pass the date object
> to the Date constructor (because I don't understand why it works), so I'd
> modify Jonas' suggestion to this:
> 
>   new aCtxt.Date(aObject.getTime())

|new Date(new Date())| seems working in my JS console although Date() doesn't accept a Date object as parameter. I guess the Date object is implicitly converted to a string value, though.

Anyway, I still got another problem at comment #12 that we cannot use the |aCtxt| to construct a Date object subjecting to that context. Any inputs are highly appreciated.

Comment 14

[Gene Lian (I already quit Mozilla)]

(In reply to Jonas Sicking (:sicking) from comment #7)
> And while you are here. Can you change
> 
> return new aCtxt.Blob([aObject]);
> 
> to
> 
> return new aCtxt.Blob([aObject], { type: aObject.type });

If you don't mind, I'd prefer addressing another bug to do this, which is a separate issue and can help others tracing the blame. Fire bug 821977.

Comment 15

[Gene Lian (I already quit Mozilla)]

(In reply to Gene Lian [:gene] from comment #12)
> After some experiments, that's not lucky we can do this like new |new
> aCtxt.Blob(...)| or |new aCtxt.File(...)|. The log returns me:
> 
>   "aCtxt.Date is not a constructor"
> 
> The |aCtxt| is passed by the |aWindow| of
> |nsIDOMGlobalPropertyInitializer.init(aWindow)| in the AlarmsManager.js [1].
> Using |this._window| inherited from |DOMRequestIpcHelper| doesn't work
> either (which seems the same window). May I have your suggestions or
> workarounds regarding this issue please?

Responds to myself: following [1] to implement a Cu.createDateIn() might be an option but I'm not sure if there exists any other better ways?

[1] http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/idl/xpccomponents.idl#301

Comment 16

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Try doing 

x = new aCtxt.wrappedJSObject.Date(Date.getTime());


The downside with doing that is that the page can override the "Date" property and can return an arbitrary thing, or run arbitrary JS when the above is called. But that's very unlikely that as far as I can see it can't cause any harm by doing so.

But we should file a followup on adding a Cu.createDateIn function.

Comment 17

[Gene Lian (I already quit Mozilla)]

Thanks Jonas! Good to learn the trick!

I think I can directly add a Cu.createDateIn(), which seems a simple work. ;)

Comment 18

[Gene Lian (I already quit Mozilla)]

Attachment: Part 1, ObjectWrapper.jsm and Cu.createDateIn(), V2

Comment 19

[Gene Lian (I already quit Mozilla)]

Try: https://tbpl.mozilla.org/?tree=Try&rev=6b441d762f52

Comment 20

[[:fabrice] Fabrice Desré]

Comment on attachment 692819 [details] [diff] [review]
Part 1, ObjectWrapper.jsm and Cu.createDateIn(), V2

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

::: js/xpconnect/idl/xpccomponents.idl
@@ +306,5 @@
> +     * Returns a date given timestamp |msec| created in |vobj|'s compartment.
> +     */
> +    [implicit_jscontext]
> +    jsval createDateIn(in jsval vobj, in long long msec);
> +

Nit: update the UUID of the interface.

Comment 21

[Gene Lian (I already quit Mozilla)]

Attachment: Part 1, ObjectWrapper.jsm and Cu.createDateIn(), V2.1

Updated the UUID. Thanks Fabrice for the reminder!

Comment 22

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 692840 [details] [diff] [review]
Part 1, ObjectWrapper.jsm and Cu.createDateIn(), V2.1

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

Holy moly this is awesome! Thanks for doing the extra work here!

Comment 23

[Gene Lian (I already quit Mozilla)]

(In reply to Gene Lian [:gene] from comment #19)
> Try: https://tbpl.mozilla.org/?tree=Try&rev=6b441d762f52

All green!

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/be80d621f9bb
https://hg.mozilla.org/integration/mozilla-inbound/rev/c4332c6f071d

Comment 25

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/be80d621f9bb
https://hg.mozilla.org/mozilla-central/rev/c4332c6f071d

Comment 26

[Aaron Train [:aaronmt]]

Is there any chance of this uplifting to mozilla-beta?

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/3e0daaa7f652
https://hg.mozilla.org/releases/mozilla-aurora/rev/ef029ee6e863

https://hg.mozilla.org/releases/mozilla-b2g18/rev/0f6756619343
https://hg.mozilla.org/releases/mozilla-b2g18/rev/7af25dbabfbd