Closed Bug 1026737 Opened 6 years ago Closed 6 years ago

[1.4 => 2.0][Vertical Homescreen] Bookmarks are erased after an OTA update

Categories

(Core Graveyard :: DOM: Apps, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(blocking-b2g:1.3T+, firefox31 wontfix, firefox32 fixed, firefox33 fixed, b2g-v1.3T fixed, b2g-v1.4 fixed, b2g-v2.0 fixed, b2g-v2.1 fixed)

RESOLVED FIXED
mozilla33
blocking-b2g 1.3T+
Tracking Status
firefox31 --- wontfix
firefox32 --- fixed
firefox33 --- fixed
b2g-v1.3T --- fixed
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed
b2g-v2.1 --- fixed

People

(Reporter: jlorenzo, Assigned: amac)

References

Details

(Keywords: dataloss, regression, Whiteboard: [systemsfe])

Attachments

(1 file, 2 obsolete files)

Steps to reproduce
1. Flash your device to the last 1.4 nightly build.
1bis. (Workaround) Change the channel to aurora with that script https://github.com/nhirata/B2G-flash-tool/blob/new_channels/change_channel.sh
2. Add a bookmark to homescreen
3. Upgrade via OTA

Actual result
No more bookmark displayed even after a reboot.
Standard user path.
blocking-b2g: --- → 2.0?
QA Whiteboard: [VH-FL-blocking-][VH-FC-blocking+]
See Also: → 1017069
blocking-b2g: 2.0? → 2.0+
Keywords: dataloss, regression
Cristian - could you take a look at this?
Flags: needinfo?(crdlc)
Flags: needinfo?(crdlc)
Carmen and me are working on it
Hi Fabrice, Kevin,

   After testing Carmen and me this issue we detected a problem with navigator.mozApps.getSelf(). This method is not performed after calling the first time. I mean the second call or consecutive are not working. So the migration process is not started [1].

   I found this problem in vertical homescreen and I fixed it doing this call in a singleton [2] and reusing the app object in different places.

   I would like to know if we cannot call to navigator.mozApps.getSelf() several times by design. If we cannot do it, we should follow the same approach taken in vertical homescreen. Otherwise, we should move the bug to Apps component thought.

[1] https://github.com/mozilla-b2g/gaia/blob/v2.0/apps/ftu/js/tutorial.js#L23
[2] https://github.com/mozilla-b2g/gaia/blob/v2.0/apps/verticalhome/js/app_manager.js#L46
Flags: needinfo?(kgrandon)
Flags: needinfo?(fabrice)
I told ferjm some time ago that this was incorrect (although I believed that was only on his patch):

https://mxr.mozilla.org/mozilla-central/source/dom/apps/src/Webapps.js#63

The way it's now, if we have several requests pending, only one of them will succeed. That is, if we have (easiest test):

mozApps.getSelf().onsuccess=callback1;
mozApps.getSelf().onsuccess=callback2;

Either callback1 or callback2 will be executed, but not both (and yet the parent process will have executed the getSelf process twice).

That's a bug :)
Assignee: nobody → amac
QA Whiteboard: [VH-FL-blocking-][VH-FC-blocking+] → [VH-FL-blocking+][VH-FC-blocking+]
Data migration is not a FL blocker.
QA Whiteboard: [VH-FL-blocking+][VH-FC-blocking+] → [VH-FL-blocking-][VH-FC-blocking+]
Duplicate of this bug: 994079
As I mentioned at https://bugzilla.mozilla.org/show_bug.cgi?id=994079#c4 I would fix this at DOMRequestIPCHelper if possible.
Antonio already answered this
Flags: needinfo?(kgrandon)
Flags: needinfo?(fabrice)
Component: Gaia::Homescreen → DOM: Apps
Product: Firefox OS → Core
Attached patch WIP, just for feedback (obsolete) — Splinter Review
This is a WIP (should be complete but cannot vouch for it) just to show the general idea. It follow ferjm suggestion of implementing the listener counter  on DOMRequestIPCHelper.
Attachment #8442181 - Flags: feedback?(ferjmoreno)
Attachment #8442181 - Flags: feedback?(fabrice)
Comment on attachment 8442181 [details] [diff] [review]
WIP, just for feedback

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

Actually, are there any cases where we don't want to count listeners?
Attachment #8442181 - Flags: feedback?(fabrice) → feedback+
(In reply to < away until June 17 > from comment #11)
> Comment on attachment 8442181 [details] [diff] [review]
> WIP, just for feedback
> 
> Review of attachment 8442181 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Actually, are there any cases where we don't want to count listeners?

Hmm... After actually checking it (and not just assuming it :)) I see now that addMessageListeners and removeMessageListeners are used only on Webapps (and on the unit test!) and there we want to count the listeners always. So I'll remove the boolean on the final version.
Comment on attachment 8442181 [details] [diff] [review]
WIP, just for feedback

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

LGTM without the boolean
Attachment #8442181 - Flags: feedback?(ferjmoreno) → feedback+
I've done some in device testing and modified the tests.

Try run is at https://tbpl.mozilla.org/?tree=Try&rev=b258bc620210

Carmen, can you check that this fixes your problem with getSelf() also?
Attachment #8442181 - Attachment is obsolete: true
Attachment #8442803 - Flags: review?(fabrice)
Flags: needinfo?(cjc)
Blocks: 1020158
Attachment #8442803 - Flags: review?(fabrice) → review+
Comment on attachment 8442803 [details] [diff] [review]
V1. Implement listener counting on DOMRequestHelper

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

Actually, test_domrequesthelper.xul is not happy at all on try.
Attachment #8442803 - Flags: review+
My bad... There was a next on a catch on the test that should have been on a try, and didn't notice locally cause I thought that when running a single test the test not ending was the normal behavior (at least it was some months ago).

Only the test changed.

Fixed now, hopefully. Try run only for the previously failing tests is at:

https://tbpl.mozilla.org/?tree=Try&rev=581aadf3ada9
Attachment #8442803 - Attachment is obsolete: true
Attachment #8443035 - Flags: review?(fabrice)
Comment on attachment 8443035 [details] [diff] [review]
V2. Implement listener counting on DOMRequestHelper. Fixed test.

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

green try so far, thanks. I'll land once all tests are done.
Attachment #8443035 - Flags: review?(fabrice) → review+
I have tried applying this patch and it works perfectly when migration from 1.3 -> master, 1.4 -> master, 1.3 -> 2.0 and 1.4 -> 2.0
Flags: needinfo?(cjc)
https://hg.mozilla.org/mozilla-central/rev/acf7a09d3f45
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Does this affect v1.3t?

something like https://bugzilla.mozilla.org/show_bug.cgi?id=1010690#c88
Flags: needinfo?(jlorenzo)
(In reply to ying.xu from comment #21)
> Does this affect v1.3t?
> 
> something like https://bugzilla.mozilla.org/show_bug.cgi?id=1010690#c88

Yes, that's the same issue.
Fabrice answered the question.
Flags: needinfo?(jlorenzo)
Blocks: 1010690
Fabrice, do we need land this patch on v1.3t?
(In reply to James Zhang (Spreadtrum) from comment #24)
> Fabrice, do we need land this patch on v1.3t?

No, this is with the new homescreen, 1.3t is unaffected.
Ha, my bad. The title is misleading, that's about the getSelf() calls. So yes, we should take that on 1.3t.
Somebody help to uplift this to 1.3t?
blocking-b2g: 2.0+ → 1.3T?
Fabrice, should we ask an approval and land this in v1.4 as well?
Flags: needinfo?(fabrice)
(In reply to Julien Wajsberg [:julienw] (away June 21 to 30) from comment #30)
> Fabrice, should we ask an approval and land this in v1.4 as well?

That's up to 1.4 owners. Ying, feel free to ask for 1.4 approval!
Flags: needinfo?(fabrice) → needinfo?(ying.xu)
NOTE: This flag is now for security issues only. Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): mozapp.getself()
User impact if declined:  Calling mozApps.getSelf() twice, the second call failed
Testing completed:  call mozApps.getSelf() twice continuously
Risk to taking this patch (and alternatives if risky):  unknown
String or UUID changes made by this patch:

and it seems I don't have the rights to set Flags: approval-mozilla-b2g30
so I set block 1.4? instead
blocking-b2g: 1.3T+ → 1.4?
Flags: needinfo?(ying.xu)
Needinfo Lawrence if there's an issue with being unable to set the flag. The blocking flag is set correctly here, so I'm reverting the blocking flag.
blocking-b2g: 1.4? → 1.3T+
Flags: needinfo?(lmandel)
Comment on attachment 8443035 [details] [diff] [review]
V2. Implement listener counting on DOMRequestHelper. Fixed test.

>User impact if declined:  Calling mozApps.getSelf() twice, the second call >failed
The bug description has a better summary of the user impact.

>Testing completed:  call mozApps.getSelf() twice continuously
I see that automated tests have been included with the patch.

>Risk to taking this patch (and alternatives if risky):  unknown
I'm not pleased with this response but as this has been on 1.3T for several days and this being requested by Ying I'm willing to approve without further discussion on this point.

> String or UUID changes made by this patch:
I did a quick scan of the patch and see no string or UUID changes.

>and it seems I don't have the rights to set Flags: approval-mozilla-b2g30
>so I set block 1.4? instead
Approvals are set on the patch. Can you confirm that you can't set the approval request when viewing the patch details?
Attachment #8443035 - Flags: approval-mozilla-b2g30+
Flags: needinfo?(lmandel)
(In reply to Lawrence Mandel [:lmandel] from comment #34)

> >and it seems I don't have the rights to set Flags: approval-mozilla-b2g30
> >so I set block 1.4? instead
> Approvals are set on the patch. Can you confirm that you can't set the
> approval request when viewing the patch details?

thanks. I found that.
Blocks: 1036868
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.