842981 - For DOMRequest whose result is an array, Array.isArray(request.result) will return false.

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Feb.7]

File this bug for gaia as I still can pass marionette tests for gecko.

my gecko is from latest m-c  changeset:122284:0577eb1893c4

I found out that now 'Import SIM Contact" is broken
After some traces I found out in [1], 
the check of Array.isArray(self.items) is false so the importSlice cannot be called.

The gaia UI changed is done in Bug 835555,
However the gaia commit works well in gecko b2g18 branch, not sure why it's broken in latest m-c.

[1] https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/contacts/js/utilities/import_sim_contacts.js#L40

Comment 1

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Feb.7]

CCing bholley and peterv

Hi Bob and Peter
I found in latest m-c, if an API returning DOMRequest, and the 'result' is an array, for example, [obj1, obj2, ..], and Array.isArray(request.result) will be 'false'.

However in b2g18 branch, the Array.isArray(request.result) is true,

do you have any idea something changed in recent m-c ?

Comment 2

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Feb.7]

To be more clear

Chrome : isArray -> returns true
Content: isArray -> returns true
Gaia:    isArray -> returns false

The bridge between Content and Gaia is DOMRequest.

ContactManager.js, DOMRequest.fireSuccess(result)  -->  (Gaia)  onsuccess() {...}

Comment 3

[Bobby Holley (:bholley)]

How is this object being exposed to content? If it's just a chrome object with a cross-compartment wrapper, Array.isArray will return false, because the JSClass check will fail on the proxy here:

http://mxr.mozilla.org/mozilla-central/source/js/src/jsarray.cpp#2474

The long-term solution here is to expose these objects with WebIDL, though that probably doesn't help you here. I'm guessing the "Content" case works because somebody is cloning the object in content (perhaps with Cu.createArrayIn), which causes it to be a bonafide content Array. There's a good chance ObjectWrapper.jsm is involved.

Comment 4

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Feb.7]

Hi ,Bobby

Thanks for the tip, after some traces I found the check IsObjectWithClass in jsobjinlines.h

http://mxr.mozilla.org/mozilla-central/source/js/src/jsobjinlines.h#1793

In m-c, it would take the RootedObject in line 1797 to call ObjectClassIs,
this is done in Bug 839376,
so that's why in m-c Array.isArray will retunn false, however in b2g18 branch it still retunrs true.

b2g18 src:
https://git.mozilla.org/?p=releases/gecko.git;a=blob;f=js/src/jsobjinlines.h;h=28e75dc45cf48836ee6576b24dfa4c1396c03740;hb=refs/heads/gecko-18#l1612


For your question,
The array is passed to content (ContactManager.js) by IPC in javascript
(childprocessmessagemanager and parentprocessmessagemanager)

The returned array is passed to Gaia by DOMRequest

http://mxr.mozilla.org/mozilla-central/source/dom/contacts/ContactManager.js#457

The IDL for that API is in nsIDOMContactManager.idl

http://mxr.mozilla.org/mozilla-central/source/dom/interfaces/contacts/nsIDOMContactManager.idl#39

The returned Array will be stored in DOMRequest.result.

One question about the WebIDL,
how does this API look like in WebIDL to avoid this kind of problem?
I mean how does WebIDL specify the result in DOMRequest would be type of Array?

thanks

Comment 5

[Bobby Holley (:bholley)]

(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #4)
> Hi ,Bobby
> 
> Thanks for the tip, after some traces I found the check IsObjectWithClass in
> jsobjinlines.h
> 
> http://mxr.mozilla.org/mozilla-central/source/js/src/jsobjinlines.h#1793
> 
> In m-c, it would take the RootedObject in line 1797 to call ObjectClassIs,
> this is done in Bug 839376,
> so that's why in m-c Array.isArray will retunn false, however in b2g18
> branch it still retunrs true.
> 
> b2g18 src:
> https://git.mozilla.org/?p=releases/gecko.git;a=blob;f=js/src/jsobjinlines.h;
> h=28e75dc45cf48836ee6576b24dfa4c1396c03740;hb=refs/heads/gecko-18#l1612

What does rooting have to do with it? That change should be entirely transparent to consumers.

I'm almost positive the difference you're seeing is due to this changeset:
https://hg.mozilla.org/integration/mozilla-inbound/rev/eae731552b88#l2.34

In particular, we used to let ObjectClassIs see through security wrappers, and now we don't (we decided not to backport this to b2g18).
 
> One question about the WebIDL,
> how does this API look like in WebIDL to avoid this kind of problem?
> I mean how does WebIDL specify the result in DOMRequest would be type of
> Array?

I believe that the long term plan is for things like DOMRequest.result, which currently use a raw jsval, to use a WebIDL |any| or some other variant type, and let it be taken care of by the binding code. We don't yet have the infrastructure to do that though, I don't think. :-(

Comment 6

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Feb.7]

Hi, Bobby
Yes, you're correct. Thanks for the detail information.
So I'll close this bug as INVALID/

Hi, Jose H. Cantera
I will file another bug for the gaia side to remove the Array.isArray check.

thanks

Comment 7

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Feb.7]

(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #6)
> Hi, Jose H. Cantera
> I will file another bug for the gaia side to remove the Array.isArray check.
> 
Bug 849730.

Comment 8

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

Array.isArray is working properly here. However if you use ObjectWrapper.jsm to clone the array into the correct window before calling requestservice.fireSuccess, then Array.isArray will return true and things will work as you expect.

Comment 9

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Feb.7]

Jonas, thanks for the tips. It's really helpful. 

Bholley amd Gregor,
should I back out the patch from Bug 849730 and upload a patch with ObjectWrapper in this bug?

thanks

Comment 10

[Bobby Holley (:bholley)]

(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #9)
> Jonas, thanks for the tips. It's really helpful. 
> 
> Bholley amd Gregor,
> should I back out the patch from Bug 849730 and upload a patch with
> ObjectWrapper in this bug?

My impression is that we want to leave the patch for bug 849730 in place to reduce regression risk. I'm not sure what Gregor wants to do about this bug, though.

Comment 11

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

Either way would should probably to the ObjectWrapper thing. I think the array will behave strangely otherwise, for example i think you can't set arbitrary expandos on it or otherwise modify it.

Comment 12

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Feb.7]

Thanks for Bobby's and Jonas' comments.
I'll upload a quick patch for this bug. 

For Bug 849730, I'll leave that to Bobby to decide.

Comment 13

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Feb.7]

Attachment: Part 1: Add ObjectWrapper to wrap array object.

Comment 14

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Feb.7]

Attachment: Part 2: marionette test for Array.isArray.

Comment 15

[Mozilla RelEng Bot]

Try run for 41d6329560d5 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=41d6329560d5
Results (out of 4 total builds):
    success: 4
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-41d6329560d5

Comment 16

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Feb.7]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dbb1f97672f0
https://hg.mozilla.org/integration/mozilla-inbound/rev/d10ac6c5b14f

Comment 17

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Feb.7]

Sorry, I found out I pushed Part 2 first.
So I backed out and re-push them again.

Sorry for my mistake again.


https://hg.mozilla.org/integration/mozilla-inbound/rev/7687d195032a
https://hg.mozilla.org/integration/mozilla-inbound/rev/89d9397250fd
https://hg.mozilla.org/integration/mozilla-inbound/rev/f80f26a81645
https://hg.mozilla.org/integration/mozilla-inbound/rev/cf6c46098cc6

Comment 18

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/f80f26a81645
https://hg.mozilla.org/mozilla-central/rev/cf6c46098cc6