849730 - [Contacts] Make Array.isArray work for contacts

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Contacts, defect)

Description

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

From Bug 842981, I found that Array.isArray check in 

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

will become false on m-c.

After some discussion with bholley (see Bug 842981),
he commented this is the correct behavior.

So I think for now, we should remove that isArray check in gaia,
otherwise "Import SIM Contacts" is broken now.

Hi Jose M Cantera,
Do you think we could remove the isArray check from your previous patch?

thanks

Comment 1

[Jose Manuel Cantera]

-1 to change that. Array.isArray should work properly otherwise the platform side is broken and should be fixed. 

thanks

Comment 2

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

CCing bholley and Gregor
I am not an expert from JS engine, hope bholley and Gregor could share more comments here.
(Also the changes to JS Engine is from Bug 809652, however I cannot access it).

Comment 3

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(In reply to Jose M. Cantera from comment #1)
> -1 to change that. Array.isArray should work properly otherwise the platform
> side is broken and should be fixed. 
> 
> thanks

Hi, Jose

If you still think 'platfom side is broken', please re-open Bug 842981 and discuss that implementation detail with bholley.

Also from nsIDOMContactManager.idl, it doesn't define specifically the 'result' in DOMRequest is type of Array, in order to make your patch has more compatibility, I think we should remove Array.isArray check.

Array.isArray in your pevious patch could work on b2g18 because the code is programmed to 'implementation', not to interface.

How do you think?

Thanks

Comment 4

[Bobby Holley (:bholley)]

(In reply to Jose M. Cantera from comment #1)
> -1 to change that. Array.isArray should work properly otherwise the platform
> side is broken and should be fixed. 
> 
> thanks

The issue is that wrapped chrome JS-objects aren't actually a good way to expose DOM APIs, but in some of the rush to get B2G out the door we ended up doing that anyway.

We could special-case the objectClassIs to work for these kinds of wrappers if there's an associated __exposedProps__ or something like that. But this API is deprecated at this point, and we don't really want to alter the semantics for non-security reasons. Unless it's a big pain to change this in the contact manager, that's probably the path of least resistance.

Comment 5

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

ping? Jose M. Cantera

Comment 6

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Pull Request

Comment 7

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 724415 [details]
Pull Request

Hi, Kaze 
Can you review this for me?

The background here is explained in comment 4 and Bug 842981 from bholley.

So in this patch I just removed Array.isArray check for the result returned from DOMRequest.

Thanks

Comment 8

[Gregor Wagner [:gwagner]]

(In reply to Jose M. Cantera from comment #1)
> -1 to change that. Array.isArray should work properly otherwise the platform
> side is broken and should be fixed. 
> 
> thanks

I agree here.
This should be fixed with bug 850430 but I don't see this happening in the next few days. We can take this PR as a quick fix but I am worried that we have more bugs like this in gaia now.

Comment 9

[Bobby Holley (:bholley)]

If you think this is a significant source of potential regressions, I can hack the platform to make this work.

Comment 10

[Gregor Wagner [:gwagner]]

(In reply to Bobby Holley (:bholley) from comment #9)
> If you think this is a significant source of potential regressions, I can
> hack the platform to make this work.

Lets start with the behavior that changed and I can take a look if we have to fix more code.

So the current use-case we want to fix is an array that is created when we send the imported SIM contacts from the parent to the child via the parent/childprocessmessagemanager. I believe here we create the array somewhere in the message manager code in c++, modify the content in ContactManager.js and hand it out to content. So this stopped working.

How about an array that is created in JS chrome code and handed out to content? Does this still work? If I read your comment https://bugzilla.mozilla.org/show_bug.cgi?id=842981#c3 correctly it also stopped working. We might have more code that does this but I have to check.

Comment 11

[Bobby Holley (:bholley)]

(In reply to Gregor Wagner [:gwagner] from comment #10)
> How about an array that is created in JS chrome code and handed out to
> content? Does this still work? If I read your comment
> https://bugzilla.mozilla.org/show_bug.cgi?id=842981#c3 correctly it also
> stopped working. We might have more code that does this but I have to check.

That comment wasn't entirely accurate. In reality, there's a proxy hook called "objectClassIs" that will be invoked if we ever hit a cross-compartment wrapper. In bug 809652, I made that hook always return false for security wrappers, mostly as defense-in-depth. This runs up, yet again, into the fundamental problem we're having with COWs: they're security wrappers, and we need to protect the wrappee, but people expect them to be transparent like bonafide content objects.

I can make a special-case override for COWs in XPConnect that lets the ObjectClassIs trap through. It's kind of gross, but I think it should be ok security-wise so long as we commit to eventually moving contacts over to WebIDL. Shall I go ahead and do that?

Comment 12

[Gregor Wagner [:gwagner]]

(In reply to Bobby Holley (:bholley) from comment #11)
> 
> I can make a special-case override for COWs in XPConnect that lets the
> ObjectClassIs trap through. It's kind of gross, but I think it should be ok
> security-wise so long as we commit to eventually moving contacts over to
> WebIDL. Shall I go ahead and do that?


I think this would be the better workaround. We shouldn't change gaia code.
Reuben started working on the WebIDL implementation yesterday but I was already told that we might not have all the features we need in place right now. So it might take some time.
Thanks!

Comment 13

[Bobby Holley (:bholley)]

Attachment: Let objectClassIs through for COWs. v1

Comment 14

[Bobby Holley (:bholley)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/030a2c329765

Comment 15

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/030a2c329765