Closed Bug 991897 Opened 6 years ago Closed 6 years ago

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

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(blocking-b2g:1.3T+, b2g-v1.3 unaffected, b2g-v1.3T verified)

RESOLVED FIXED
1.4 S6 (25apr)
blocking-b2g 1.3T+
Tracking Status
b2g-v1.3 --- unaffected
b2g-v1.3T --- verified

People

(Reporter: rpribble, Assigned: crdlc)

Details

(Whiteboard: [tarako-exploratory], OOM, [systemsfe], 1.3tarakorun2)

Attachments

(4 files)

Attached file 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
Attached file 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.
Attached audio HomescreenIcon.ogg
blocking-b2g: --- → 1.3T?
Component: Gaia::Browser → Gaia::Homescreen
broken feature on tarako, 1.3T+
Gregor, can we have someone on your team to look at this? thanks
blocking-b2g: 1.3T? → 1.3T+
Flags: needinfo?(anygregor)
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.
Whiteboard: [tarako-exploratory] → [tarako-exploratory], OOM
Flags: needinfo?(anygregor)
Whiteboard: [tarako-exploratory], OOM → [tarako-exploratory], OOM, [systemsfe]
ni? ehung for initial investigation before Gregor's team distribute tarako devices to more devs
Flags: needinfo?(ehung)
Whiteboard: [tarako-exploratory], OOM, [systemsfe] → [tarako-exploratory], OOM, [systemsfe], 1.3tarakorun2
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.
Flags: needinfo?(ehung) → needinfo?(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.
Flags: needinfo?(crdlc)
Gregor, is your team available for this blocking bug?
Flags: needinfo?(anygregor)
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
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → WORKSFORME
Flags: needinfo?(anygregor)
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
Status: RESOLVED → REOPENED
Flags: needinfo?(jschmitt)
Resolution: WORKSFORME → ---
Flags: needinfo?(anygregor)
(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).
Flags: needinfo?(jschmitt)
Status: REOPENED → NEW
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 ?
Flags: needinfo?(dale)
(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?
Flags: needinfo?(anygregor)
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)
Assignee: nobody → dale
Flags: needinfo?(dale)
You don't really need a device to simulate the cause. You could just kill the home screen process at step 3 with adb.
Target Milestone: --- → 1.4 S6 (25apr)
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
(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.
Flags: needinfo?(pdolanjski)
How did we do it on 1.4?
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
If the homescreen is getting killed and decoupling is difficult, would it be an option to disable the feature completely?
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
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
Upps I forgot the :ni
Flags: needinfo?(dale)
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?
Status: NEW → ASSIGNED
Flags: needinfo?(timdream)
Brian, can you team help? see comment 10 for STR. Thanks.
Flags: needinfo?(timdream) → needinfo?(brhuang)
Al, Could you or our member to check with comment 10 for STR? Thanks.
Flags: needinfo?(brhuang) → needinfo?(atsai)
I didnt realise the data managed to get into indexeddb, that fix makes sense / looks good though. Reassigning
Assignee: dale → crdlc
Flags: needinfo?(dale)
Attached file Patch v1 for v1.3t
Thanks for this review
Attachment #8409670 - Flags: review?(dale)
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
Attachment #8409670 - Flags: review?(dale) → review+
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
Flags: needinfo?(pdolanjski)
How should we proceed? ask for approval to Fabrice?
you may land in 1.3T as this bug has 1.3T+. thanks
ok, addressing comments on github and after travis green merging, thanks a lot
Merged in v1.3t:

https://github.com/mozilla-b2g/gaia/commit/c6092e7b9dc103d9f7ac326081f1e46b347bb6d6
Status: ASSIGNED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Flags: needinfo?(atsai)
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
You need to log in before you can comment on or make changes to this bug.