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

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

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

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

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.

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

(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

(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

(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.

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

(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.

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");

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.

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.

(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?

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

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

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 on attachment 8346737 [details] [diff] [review] fix I am pushing this to try now, but what you think of this fix?

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

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.

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 on attachment 8349451 [details] [diff] [review] fix-slice v1 r=me