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

RESOLVED FIXED in Firefox 29, Firefox OS v1.4

Status

()

Core
JavaScript Engine
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Darren, Assigned: evilpie)

Tracking

({regression, smoketest})

29 Branch
mozilla29
ARM
Gonk (Firefox OS)
regression, smoketest
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:1.4+, firefox29 fixed, b2g-v1.2 unaffected, b2g-v1.3 unaffected, b2g-v1.3T unaffected, b2g-v1.4 fixed)

Details

Attachments

(4 attachments)

34.72 KB, image/jpeg
Details
13.25 KB, application/zip
Details
fix
2.19 KB, patch
Bobby Holley (parental leave - send mail for anything urgent)
: feedback+
Details | Diff | Splinter Review
3.30 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

4 years ago
Created attachment 8346195 [details]
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
(Reporter)

Comment 1

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

Updated

4 years ago
blocking-b2g: --- → 1.4?
Keywords: regression, regressionwindow-wanted, smoketest

Updated

4 years ago
QA Contact: mvaughan

Comment 2

4 years ago
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
status-b2g-v1.2: --- → unaffected
status-b2g-v1.3: --- → unaffected
Keywords: regressionwindow-wanted
Hi Francisco, can you take a look at this? thanks
Flags: needinfo?(francisco.jordano)
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.
Blocks: 936250
Depends on: 949214

Updated

4 years ago
Depends on: 949415

Comment 5

4 years ago
I have just opened 949415 as this has been caused by a recent platform regression in mozContacts or related infra.
Flags: needinfo?(francisco.jordano)

Comment 6

4 years ago
(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

Updated

4 years ago
No longer depends on: 949214
(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.

Updated

4 years ago
Blocks: 697343
No longer blocks: 936250

Comment 9

4 years ago
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.

Updated

4 years ago
No longer depends on: 949415
Duplicate of this bug: 949415

Updated

4 years ago
Component: Gaia::Contacts → DOM: Contacts
Product: Firefox OS → Core
Version: unspecified → 29 Branch

Updated

4 years ago
Depends on: 946294
Created attachment 8346673 [details]
Test App
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.
Blocks: 931711
tracking-firefox29: --- → ?
(Assignee)

Comment 15

4 years ago
Created attachment 8346737 [details] [diff] [review]
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.

Updated

4 years ago
Component: DOM: Contacts → JavaScript Engine
(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?
Flags: needinfo?(bzbarsky)
> This testcase won't test this regression after bug 946294 is fixed, right?

Yes, correct.
Flags: needinfo?(bzbarsky)
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.
Flags: needinfo?(evilpies)
(Assignee)

Comment 20

4 years ago
Comment on attachment 8346737 [details] [diff] [review]
fix

I am pushing this to try now, but what you think of this fix?
Attachment #8346737 - Flags: feedback?(bobbyholley+bmo)
Flags: needinfo?(evilpies)
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.
Attachment #8346737 - Flags: feedback?(bobbyholley+bmo) → feedback+
(Assignee)

Updated

4 years ago
Assignee: nobody → evilpies
(Assignee)

Comment 23

4 years ago
Created attachment 8349451 [details] [diff] [review]
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
Attachment #8349451 - Flags: review?(bzbarsky)
Comment on attachment 8349451 [details] [diff] [review]
fix-slice v1

r=me
Attachment #8349451 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 25

4 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/ec79d33b9b0b
https://hg.mozilla.org/mozilla-central/rev/ec79d33b9b0b
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29

Updated

4 years ago
blocking-b2g: 1.4? → 1.4+

Updated

4 years ago
status-firefox29: affected → fixed
tracking-firefox29: ? → ---
status-b2g-v1.3T: --- → unaffected
status-b2g-v1.4: --- → fixed
You need to log in before you can comment on or make changes to this bug.