949197 - [B2G][Contacts] Facebook contacts imported, display "Undefined" when editing.

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Darren]

Attachment: undefined contact names

Repro Steps:
1. Load Buri Build: 20131211040203
2. Have wifi or cell data turned on
3. Open contacts app
4. Tap on Setting icon
5. Toggle on Sync friends
6. Enter valid Facebook
7. Import contacts
8. Tap on one of the contacts imported and select the edit icon

Actual Results:
The names of the contact display as "Undefined".

Expected Results:
All contacts will display the correct information when being edited.

Environmental Variables
Device: (example:Leo v 1.1.0 Mozilla/ COM RIL)
Build ID: 20131211040203
Gecko: http://hg.mozilla.org/mozilla-central/rev/12ea03a70243
Gaia: 6415b8b44068596404c10365394544e94edd5ce5
Platform Version: 29.0a1
Base Build: 20131115

Comment 1

[Darren]

The device checked on was -
Device: Buri Master M-C MOZ RIL

Comment 2

[Matthew Vaughan]

This issue only reproduces on the build specified in comment 0 & comment 1.

This issue does NOT reproduce on the 12/10 Buri Master M-C (1.4) build or on the 12/11 1.2 & 1.3 builds.

- Works -
Environmental Variables:
Device: Buri Master M-C MOZ RIL
BuildID: 20131210040206
Gaia: c952e2756c03eceb4de6a3eba15651741a62f9e8
Gecko: df82be9d89a5
Version: 29.0a1
Firmware Version: 20131115

- Broken -
Environmental Variables:
Device: Buri Master M-C MOZ RIL
BuildID: 20131211040203
Gaia: 6415b8b44068596404c10365394544e94edd5ce5
Gecko: 12ea03a70243
Version: 29.0a1
Firmware Version: 20131115

Comment 3

[Joe Cheng [:jcheng] (please needinfo)]

Hi Francisco, can you take a look at this? thanks

Comment 4

[Jason Smith [:jsmith]]

There's four commits in the Gaia regression range touching the contacts app:

1. https://github.com/mozilla-b2g/gaia/commit/8e7ec254e30b09388a890827dfb95f55b641625c
2. https://github.com/mozilla-b2g/gaia/commit/981000da043d6c2bd5e258b512134386f4b52d54
3. https://github.com/mozilla-b2g/gaia/commit/4f95bda85d060e45fb98c0b87a8e1c956392019b
4. https://github.com/mozilla-b2g/gaia/commit/dc7af55a60f67962bcee6366d2cf878e36ac9057

[1] seems unlikely as it's modifying test locales into the dev branch. [2] seems unlikely because it's dealing with exporting. [4] seems less likely since it's dealing with find duplicates functionality. That leaves [3], which is dealing with facebook linking.

On that regard, this was broken by bug 936250.

Comment 5

[Jose Manuel Cantera]

I have just opened 949415 as this has been caused by a recent platform regression in mozContacts or related infra.

Comment 6

[Jose Manuel Cantera]

(In reply to Jason Smith [:jsmith] from comment #4)
> There's four commits in the Gaia regression range touching the contacts app:
> 
> 1.
> https://github.com/mozilla-b2g/gaia/commit/
> 8e7ec254e30b09388a890827dfb95f55b641625c
> 2.
> https://github.com/mozilla-b2g/gaia/commit/
> 981000da043d6c2bd5e258b512134386f4b52d54
> 3.
> https://github.com/mozilla-b2g/gaia/commit/
> 4f95bda85d060e45fb98c0b87a8e1c956392019b
> 4.
> https://github.com/mozilla-b2g/gaia/commit/
> dc7af55a60f67962bcee6366d2cf878e36ac9057
> 
> [1] seems unlikely as it's modifying test locales into the dev branch. [2]
> seems unlikely because it's dealing with exporting. [4] seems less likely
> since it's dealing with find duplicates functionality. That leaves [3],
> which is dealing with facebook linking.
> 
> On that regard, this was broken by bug 936250.

sorry but none of them as reponsible for this regression. this has been caused by bug 949415. Please do not always blame Gaia commits, take into account that there might be Gecko commits that may cause failures in Gaia.

thanks

Comment 7

[Jason Smith [:jsmith]]

(In reply to Jose M. Cantera from comment #6)
> (In reply to Jason Smith [:jsmith] from comment #4)
> > There's four commits in the Gaia regression range touching the contacts app:
> > 
> > 1.
> > https://github.com/mozilla-b2g/gaia/commit/
> > 8e7ec254e30b09388a890827dfb95f55b641625c
> > 2.
> > https://github.com/mozilla-b2g/gaia/commit/
> > 981000da043d6c2bd5e258b512134386f4b52d54
> > 3.
> > https://github.com/mozilla-b2g/gaia/commit/
> > 4f95bda85d060e45fb98c0b87a8e1c956392019b
> > 4.
> > https://github.com/mozilla-b2g/gaia/commit/
> > dc7af55a60f67962bcee6366d2cf878e36ac9057
> > 
> > [1] seems unlikely as it's modifying test locales into the dev branch. [2]
> > seems unlikely because it's dealing with exporting. [4] seems less likely
> > since it's dealing with find duplicates functionality. That leaves [3],
> > which is dealing with facebook linking.
> > 
> > On that regard, this was broken by bug 936250.
> 
> sorry but none of them as reponsible for this regression. this has been
> caused by bug 949415. Please do not always blame Gaia commits, take into
> account that there might be Gecko commits that may cause failures in Gaia.
> 
> thanks

That's not right. bug 949415 isn't a landed patch. A regressing patch would have to be a landed patch within the regression range. What probably happened here is that bug 936250 is functionally correct, but it triggered a gecko bug under the hood. On that regard, that still makes it the regressing patch, even though the fix required on the gecko side.

If you check the gecko push log below, then you'll notice that there isn't any contacts API patch that landed in the regression range. That makes bug 949415 the prime candidate for causing this bug. That needs to be backed out until the underlying gecko bug is resolved.

http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=df82be9d89a5&tochange=12ea03a70243

Comment 8

[Jason Smith [:jsmith]]

(In reply to Jason Smith [:jsmith] from comment #7)
> (In reply to Jose M. Cantera from comment #6)
> > (In reply to Jason Smith [:jsmith] from comment #4)
> > > There's four commits in the Gaia regression range touching the contacts app:
> > > 
> > > 1.
> > > https://github.com/mozilla-b2g/gaia/commit/
> > > 8e7ec254e30b09388a890827dfb95f55b641625c
> > > 2.
> > > https://github.com/mozilla-b2g/gaia/commit/
> > > 981000da043d6c2bd5e258b512134386f4b52d54
> > > 3.
> > > https://github.com/mozilla-b2g/gaia/commit/
> > > 4f95bda85d060e45fb98c0b87a8e1c956392019b
> > > 4.
> > > https://github.com/mozilla-b2g/gaia/commit/
> > > dc7af55a60f67962bcee6366d2cf878e36ac9057
> > > 
> > > [1] seems unlikely as it's modifying test locales into the dev branch. [2]
> > > seems unlikely because it's dealing with exporting. [4] seems less likely
> > > since it's dealing with find duplicates functionality. That leaves [3],
> > > which is dealing with facebook linking.
> > > 
> > > On that regard, this was broken by bug 936250.
> > 
> > sorry but none of them as reponsible for this regression. this has been
> > caused by bug 949415. Please do not always blame Gaia commits, take into
> > account that there might be Gecko commits that may cause failures in Gaia.
> > 
> > thanks
> 
> That's not right. bug 949415 isn't a landed patch. A regressing patch would
> have to be a landed patch within the regression range. What probably
> happened here is that bug 936250 is functionally correct, but it triggered a
> gecko bug under the hood. On that regard, that still makes it the regressing
> patch, even though the fix required on the gecko side.
> 
> If you check the gecko push log below, then you'll notice that there isn't
> any contacts API patch that landed in the regression range. That makes bug
> 949415 the prime candidate for causing this bug. That needs to be backed out
> until the underlying gecko bug is resolved.
> 
> http://hg.mozilla.org/mozilla-central/
> pushloghtml?fromchange=df82be9d89a5&tochange=12ea03a70243

Meant to say - prime candidate is bug 936250.

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

Just to clarify, it seems bug 949415 was caused by bug 697343 landing on Dec 10?  Should we back out bug 697343?

Comment 10

[Jason Smith [:jsmith]]

(In reply to Ben Kelly [:bkelly] from comment #9)
> Just to clarify, it seems bug 949415 was caused by bug 697343 landing on Dec
> 10?  Should we back out bug 697343?

Let me discuss this with :evilpie more - this definitely sounds like it's a fallout from that bug.

Comment 12

[Jason Smith [:jsmith]]

Attachment: Test App

Comment 13

[Boris Zbarsky [:bzbarsky]]

Here's a simple mochitest that we should just add to our test suite so we'll have basic coverage for this stuff:

  var c = new mozContact();
  c.email = [{ type: ["foo"], value: "bar@baz" }]
  var arr = c.email;
  is(arr[0].value, "bar@baz", "Should have the right value");
  arr = arr.slice();
  is(arr[0].value, "bar@baz", "Should have the right value after slicing");

Comment 14

[Boris Zbarsky [:bzbarsky]]

And of course if someone who actually owns any of the code involved is willing to step up and write some actual automated tests, that would be _much_ appreciated.

Comment 15

[Tom S [:evilpie]]

Attachment: fix

With this patch, we call js::SliceSlowly in BaseProxyHandler::slice, but the actual fix is the call in Proxy::slice. In the case when the policy doesn't allow the VOID call, but we are not going to throw, we just execute the fallback code on the proxy.

Comment 16

[Reuben Morais [:reuben]]

(In reply to Boris Zbarsky [:bz] from comment #13)
> Here's a simple mochitest that we should just add to our test suite so we'll
> have basic coverage for this stuff:

This testcase won't test this regression after bug 946294 is fixed, right?

Comment 17

[Reuben Morais [:reuben]]

Forgot to needinfo? for comment 16:
 
> This testcase won't test this regression after bug 946294 is fixed, right?

Comment 18

[Boris Zbarsky [:bzbarsky]]

> This testcase won't test this regression after bug 946294 is fixed, right?

Yes, correct.

Comment 19

[Jason Smith [:jsmith]]

Tom - Do you have an ETA on a fix here? This is blocking a daily smoketest, so we need to either fix this asap or backout the regressing patch.

Comment 20

[Tom S [:evilpie]]

Comment on attachment 8346737 [details] [diff] [review]
fix

I am pushing this to try now, but what you think of this fix?

Comment 21

[Boris Zbarsky [:bzbarsky]]

That looks good to me, fwiw.  But please add a test per comment 13.

Comment 22

[Bobby Holley (:bholley)]

Comment on attachment 8346737 [details] [diff] [review]
fix

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

::: js/src/jsproxy.cpp
@@ +2734,5 @@
>               HandleObject result)
>  {
>      JS_CHECK_RECURSION(cx, return false);
>      BaseProxyHandler *handler = proxy->as<ProxyObject>().handler();
>      AutoEnterPolicy policy(cx, handler, proxy, JSID_VOIDHANDLE, BaseProxyHandler::GET, true);

Please add /* mayThrow = */ before the |true|.

@@ +2736,5 @@
>      JS_CHECK_RECURSION(cx, return false);
>      BaseProxyHandler *handler = proxy->as<ProxyObject>().handler();
>      AutoEnterPolicy policy(cx, handler, proxy, JSID_VOIDHANDLE, BaseProxyHandler::GET, true);
> +    if (!policy.allowed()) {
> +        if (policy.returnValue()) {

assert that there's no exception pending here.

@@ +2738,5 @@
>      AutoEnterPolicy policy(cx, handler, proxy, JSID_VOIDHANDLE, BaseProxyHandler::GET, true);
> +    if (!policy.allowed()) {
> +        if (policy.returnValue()) {
> +            return js::SliceSlowly(cx, proxy, proxy, begin, end, result);
> +        }

Ditch the braces, no?

@@ +2745,1 @@
>          return policy.returnValue();

This line needs to go away, doesn't it? I'd imagine it's going to bork your try push.

Comment 23

[Tom S [:evilpie]]

Attachment: fix-slice v1

Thanks! I kind of failed with that spurious return there ;) But the new try run looks good. https://tbpl.mozilla.org/?tree=Try&rev=afe115748d5c

Comment 24

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8349451 [details] [diff] [review]
fix-slice v1

r=me

Comment 25

[Tom S [:evilpie]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/ec79d33b9b0b

Comment 26

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ec79d33b9b0b