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

Status: RESOLVED FIXED

(Core Graveyard :: DOM: Apps, defect)

Description

[Johan Lorenzo [:jlorenzo]]

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.

Comment 1

[Johan Lorenzo [:jlorenzo]]

Standard user path.

Comment 2

[Kevin Grandon :kgrandon]

Cristian - could you take a look at this?

Comment 3

[Cristian Rodriguez (:crdlc)]

Carmen and me are working on it

Comment 4

[Cristian Rodriguez (:crdlc)]

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

Comment 5

[Antonio Manuel Amaya Calvo (:amac)]

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 :)

Comment 6

[Jason Smith [:jsmith]]

Data migration is not a FL blocker.

Comment 8

[Fernando Jiménez Moreno [:ferjm]]

As I mentioned at https://bugzilla.mozilla.org/show_bug.cgi?id=994079#c4 I would fix this at DOMRequestIPCHelper if possible.

Comment 9

[Fernando Jiménez Moreno [:ferjm]]

Antonio already answered this

Comment 10

[Antonio Manuel Amaya Calvo (:amac)]

Attachment: WIP, just for feedback

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.

Comment 11

[[:fabrice] Fabrice Desré]

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?

Comment 12

[Antonio Manuel Amaya Calvo (:amac)]

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

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 8442181 [details] [diff] [review]
WIP, just for feedback

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

LGTM without the boolean

Comment 14

[Antonio Manuel Amaya Calvo (:amac)]

Attachment: V1. Implement listener counting on DOMRequestHelper

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?

Comment 15

[[:fabrice] Fabrice Desré]

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.

Comment 16

[Antonio Manuel Amaya Calvo (:amac)]

Attachment: V2. Implement listener counting on DOMRequestHelper. Fixed test.

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

Comment 17

[[:fabrice] Fabrice Desré]

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.

Comment 18

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/integration/b2g-inbound/rev/acf7a09d3f45

Comment 19

[Carmen Jimenez Cabezas]

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

Comment 20

[Ed Morley [:emorley]]

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

Comment 21

[ying.xu]

Does this affect v1.3t?

something like https://bugzilla.mozilla.org/show_bug.cgi?id=1010690#c88

Comment 22

[[:fabrice] Fabrice Desré]

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

Comment 23

[Johan Lorenzo [:jlorenzo]]

Fabrice answered the question.

Comment 24

[James Zhang (Spreadtrum)]

Fabrice, do we need land this patch on v1.3t?

Comment 25

[[:fabrice] Fabrice Desré]

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

Comment 26

[[:fabrice] Fabrice Desré]

Ha, my bad. The title is misleading, that's about the getSelf() calls. So yes, we should take that on 1.3t.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f25a36294e5c

Comment 28

[ying.xu]

Somebody help to uplift this to 1.3t?

Comment 29

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/rev/cbd689d45109

Comment 30

[Julien Wajsberg [:julienw]]

Fabrice, should we ask an approval and land this in v1.4 as well?

Comment 31

[[:fabrice] Fabrice Desré]

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

Comment 32

[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

Comment 33

[Jason Smith [:jsmith]]

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.

Comment 34

[Lawrence Mandel [:lmandel] (use needinfo)]

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?

Comment 35

[ying.xu]

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

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/c68d57d346af