991897 - [B2G][Tarako][Browser] Adding a webpage to the homescreen is not functioning correctly

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Homescreen, defect)

Description

[Rachel Pribble [:rpribble]]

Attachment: Homescreenlogcat.txt

Description:
Attempting to add a webpage to the homescreen is not functioning correctly. The user will see the message 'Added to Homescreen', but no icon will have been placed.  Repeating the 'add to homescreen' process three times in succession resulted in an icon finally appearing on homescreen, but this is not typical. Sometimes 'add to homescreen' window closes back to browser as if added correctly, but user sees no message and no icon appears.

Repro Steps:
1) Update a Tarako to BuildID: 20140403004001
2) Tap the browser icon
3) Navigate to any webpage
4) After the webpage loads, tap the star icon
5) Select 'add to homescreen'
6) Confirm add to homescreen
7) Tap home button to observe lack of icon added to homescreen

Actual:
'Add to homescreen' option is not functioning correctly.

Expected:
All features function as intended.

v1.3t Environmental Variables:
Device: Tarako v1.3t MOZ ril
BuildID: 20140403004001
Gaia: 8d212f640014db62c622e8c939ddbf8595f00438
Gecko: ddd7ba612cfe
Version: 28.1
Firmware Version: sp8810

Notes: Unable to use Firewatch debug tool.

Repro frequency: 90%
See attached: Video, logcat, desmg log

Comment 1

[Rachel Pribble [:rpribble]]

Attachment: HomescreenDesmg.txt

This issue does not occur on the Buri v1.3 MOZ ril.

Environmental Variables:
Device: Buri v1.3 MOZ ril
BuildID: 20140401164001
Gaia: c5cd3a11e91339163b351d50769eaca0067da860
Gecko: 5045a67b47ed
Version: 28.0
Firmware Version: Settings > Device Information > More Information > Firmware revision (example.D30008m)

'Add to homescreen' feature functions as expected.

Comment 2

[Rachel Pribble [:rpribble]]

Attachment: HomescreenIcon.ogg

Comment 3

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

broken feature on tarako, 1.3T+
Gregor, can we have someone on your team to look at this? thanks

Comment 4

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

For some reason, today's build is a bit better in marking the homescreen.  I have had failure; I think it had to do with the app getting killed.

Comment 5

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

ni? ehung for initial investigation before Gregor's team distribute tarako devices to more devs

Comment 6

[Evelyn Hung]

So the problem is: 

BookmarkEditor rely on homescreen main frame to actually writing a bookmark record into database. (https://github.com/mozilla-b2g/gaia/blob/v1.3t/apps/homescreen/js/bookmark_editor.js#L55)

    var homeScreenWindow = window.open('', 'main');
    if (!homeScreenWindow)
      this.close();
    else {
      homeScreenWindow.postMessage(
        new Message(Message.Type.ADD_BOOKMARK, this.data), this.origin);
      this.onsaved();
    }

However, on tarako, homescreen app is highly possible to be killed while a wabpage is loading and it won't be re-launched because it's in background. So |homeScreenWindow| will be null, and the edit screen was closed without anything happened. (it even didn't successfully end the activity because the callback function this.onsaved() wasn't invoked)

:crdlc, is it possible we decouple BookmarkEditor and homescreen mainframe? I see on master, the problem doesn't exist, I'm wondering if we can have a quick fix here. Thanks.

Comment 7

[Cristian Rodriguez (:crdlc)]

There is not quick fix for that. This is not happening in master because of bug 898325. We are migrating add/modify/remove bookmarks functionality to a new app with its datastore independent from Homescreen so bookmarks could be shared among installed homescreens in FFOS.

Comment 8

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Gregor, is your team available for this blocking bug?

Comment 9

[Josh Schmitt [Joshs]]

I cannot repro this issue with the latest 1.3T build and the latest base image sp6821.

1.3T Tarako Environmental Variables:
Device: 1.3 Tarako
BuildID: 20140411004003
Gaia: 27a0e773e01eed74e20709bdcab6894469f42a72
Gecko: 257dd37da601
Version: 28.1
Firmware Version: sp6821

Comment 10

[Tim Guan-tin Chien [:timdream] (please needinfo)]

The cause has already been identified and if you cannot reproduce it that's because your webpage is not heavy enough to make background homescreen OOM'd.

Please retest with the steps below:

1) Update a Tarako to BuildID: 20140403004001
2) Tap the browser icon
3*) Navigate to a heavy webpage, verified the homescreen process is killed at background with |adb shell b2g-ps|.
4) After the webpage loads, tap the star icon
5) Select 'add to homescreen'
6) Confirm add to homescreen
7) Tap home button to observe lack of icon added to homescreen

Comment 11

[Josh Schmitt [Joshs]]

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #10)
> The cause has already been identified and if you cannot reproduce it that's
> because your webpage is not heavy enough to make background homescreen OOM'd.
> 
> Please retest with the steps below:
> 
> 1) Update a Tarako to BuildID: 20140403004001
> 2) Tap the browser icon
> 3*) Navigate to a heavy webpage, verified the homescreen process is killed
> at background with |adb shell b2g-ps|.
> 4) After the webpage loads, tap the star icon
> 5) Select 'add to homescreen'
> 6) Confirm add to homescreen
> 7) Tap home button to observe lack of icon added to homescreen

Ok, retested and I can repro the bug following your steps and going to imgur.com (heavy website).

Comment 12

[bhavana bajaj [:bajaj]]

Hi Dale, :gwagner mentioned given you have got the the tarako! now, and should be able to help investigate this issue, can you please take a stab at this ?

Comment 13

[Gregor Wagner [:gwagner]]

(In reply to Josh Schmitt from comment #11)
> (In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from
> comment #10)
> > The cause has already been identified and if you cannot reproduce it that's
> > because your webpage is not heavy enough to make background homescreen OOM'd.
> > 
> > Please retest with the steps below:
> > 
> > 1) Update a Tarako to BuildID: 20140403004001
> > 2) Tap the browser icon
> > 3*) Navigate to a heavy webpage, verified the homescreen process is killed
> > at background with |adb shell b2g-ps|.
> > 4) After the webpage loads, tap the star icon
> > 5) Select 'add to homescreen'
> > 6) Confirm add to homescreen
> > 7) Tap home button to observe lack of icon added to homescreen
> 
> Ok, retested and I can repro the bug following your steps and going to
> imgur.com (heavy website).

The question is now what the right behavior should be. Imagine the web-page needs all the memory we have available and we already killed all other apps including homescreen. Adding the link requires some memory, so should we kill the webpage that is currently displayed?

Comment 14

[Dale Harvey (:daleharvey)]

Will take a look at this after https://bugzilla.mozilla.org/show_bug.cgi?id=991547

If we dont have the memory to show both the bookmark activity and the website at the same time it seems like the tab process should be killed, we can reload it when the user returns, will take a look at what the process priorities are (once I have revived my device, which may take a while)

Comment 15

[Tim Guan-tin Chien [:timdream] (please needinfo)]

You don't really need a device to simulate the cause. You could just kill the home screen process at step 3 with adb.

Comment 16

[Dale Harvey (:daleharvey)]

So yeh, reproduced, the above investigation is right, there isnt really much more to investigate, if you kill the homescreen process prior to bookmarking this comes up

It works on 1.4 and master, master is expected as that is a seperate app that handles bookmarks, 1.4 I am investigating. However it seems likely that the choices will be

   1. Rewrite bookmarks to not depend on the homescreen window open
   2. Try to backport crdics bookmark app
   3. Disable add to homescreen from the browser for 1.3t

Comment 17

[Gregor Wagner [:gwagner]]

(In reply to Dale Harvey (:daleharvey) from comment #16)
> So yeh, reproduced, the above investigation is right, there isnt really much
> more to investigate, if you kill the homescreen process prior to bookmarking
> this comes up
> 
> It works on 1.4 and master, master is expected as that is a seperate app
> that handles bookmarks, 1.4 I am investigating. However it seems likely that
> the choices will be
> 
>    1. Rewrite bookmarks to not depend on the homescreen window open
>    2. Try to backport crdics bookmark app
>    3. Disable add to homescreen from the browser for 1.3t

Thanks everyone!

So our options are 1) which is super risky, 2) risky but less than 1) and 3) probably the best alternative when we compare risk and benefit.

Comment 18

[Tim Guan-tin Chien [:timdream] (please needinfo)]

How did we do it on 1.4?

Comment 19

[Dale Harvey (:daleharvey)]

The bookmarking code between 1.4 and 1.3 is pretty much the same, it still requires the window.open call but it may be a fix in gecko that allows that call to success where its failing in 1.3, trying to figure out now, I have a worry that even if we find out whats causing it to break, its still going to try loading a new frame

I am still reviving my tarako, if someone can see what happens on a 1.4 tarako that would be helpful, thanks

Comment 20

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

If the homescreen is getting killed and decoupling is difficult, would it be an option to disable the feature completely?

Comment 21

[Dale Harvey (:daleharvey)]

Thats the option Gregor and I mentioned, however before we decide I would really like to figure out why this works in 1.4 and not 1.3 despite being pretty much the same code, I finally got my tarako revived so will get on that in the morning

Comment 22

[Cristian Rodriguez (:crdlc)]

The only way what I see here is:

Scenario: Homescreen was killed while bookmarking

1) Frame should save the bookmark descriptor in a new indexedDB.
2) When Homescreen is started and rendered, it should read that indexedDB and install pending bookmarks

Does it work for you? I don't have another quick fix. Please let me know what you think about this solution. Thanks

Comment 23

[Cristian Rodriguez (:crdlc)]

Upps I forgot the :ni

Comment 24

[Cristian Rodriguez (:crdlc)]

I've just implemented my patch explained in comment 22

https://github.com/mozilla-b2g/gaia/pull/18488

Patch: https://github.com/mozilla-b2g/gaia/pull/18488.patch

Tim, could somebody of your team test it?

Comment 25

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Brian, can you team help? see comment 10 for STR. Thanks.

Comment 26

[Brian Huang [:brianhuang][:brhuang]]

Al, Could you or our member to check with comment 10 for STR? Thanks.

Comment 27

[Dale Harvey (:daleharvey)]

I didnt realise the data managed to get into indexeddb, that fix makes sense / looks good though. Reassigning

Comment 28

[Cristian Rodriguez (:crdlc)]

Attachment: Patch v1 for v1.3t

Thanks for this review

Comment 29

[Dale Harvey (:daleharvey)]

Comment on attachment 8409670 [details]
Patch v1 for v1.3t

Very nice, tested on 1.3 and confirmed the problem consistently reproduces and is fixed by this

Comment 30

[Dale Harvey (:daleharvey)]

I assumed it would be a much larger piece of work to have this work without a running homescreen, but christian has this fixed for 1.3, so I dont think we need to have a decision about removing the feature

Comment 31

[Cristian Rodriguez (:crdlc)]

How should we proceed? ask for approval to Fabrice?

Comment 32

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

you may land in 1.3T as this bug has 1.3T+. thanks

Comment 33

[Cristian Rodriguez (:crdlc)]

ok, addressing comments on github and after travis green merging, thanks a lot

Comment 34

[Cristian Rodriguez (:crdlc)]

Merged in v1.3t:

https://github.com/mozilla-b2g/gaia/commit/c6092e7b9dc103d9f7ac326081f1e46b347bb6d6

Comment 35

[Montoya Clemmons (MClemmons)]

Verified following STR from Comment 0, expected result of website icon added to homescreen occurs. 

Environmental Variables:
Device: Tarako 1.3T MOZ
BuildID: 20140429014002
Gaia: b5adc5a943d3abbd6ab070a47c847f2c24891cc5
Gecko: e9890f5d4709
Version: 28.1
Firmware Version: sp6821a_gonk4.0_user.pac