1587793 - Add PWA to Home Screen breaks on second tab

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Web Apps (PWAs), defect, P1)

Description

[Jaume Delclòs Coll]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:69.0) Gecko/20100101 Firefox/69.0

Steps to reproduce:

Close fennec completely and open it again with no tabs for a clean state.
Open any PWA, such as https://platform-status.mozilla.org/
Close the tab.
Open the PWA site again in a new tab.
Press the Add to Home icon (House with a + inside) to add it to your Home.
A modal will open with a bigger "+ ADD TO HOME SCREEN" blue button. Press that button.
You will see that the title is not "Platform Status" as stated in the manifest.json for the PWA but instead the <title> of the page, "Firefox Platform Status". This is the first symptom that the manifest is not being read correctly.
Press ADD AUTOMATICALLY or drag somewhere in the home screen.
When the icon on the home screen is pressed, "Platform Status" will open as a new tab, as if the icon were a mere link.

I have attached a video that shows first the correct behavior (without closing the tab) and then the broken behavior (as described above)

Actual results:

The resulting icon not a proper PWA but a link to open the page, as if it had been generated using:
Three dot menu -> Page -> Add Page Shortcut

Expected results:

The expected results are that the icon has the correct title and opens as a proper app. This behaviour happens when following these steps (same as before, without the tab close):

Close fennec completely and open it again with no tabs for a clean state.
Open any PWA, such as https://platform-status.mozilla.org/
Press the Add to Home icon (House with a + inside) to add it to your Home.
A modal will open with a bigger "+ ADD TO HOME SCREEN" blue button. Press that button.
You will see that the title is "Platform Status" as stated in the manifest.json for the WPA.
Press ADD AUTOMATICALLY or drag somewhere in the home screen.
When the icon on the home screen is pressed, "Platform Status" will open like an App, with no URL bar or browser UI.

Comment 1

[Jaume Delclòs Coll]

Attachment: Steps for correct and buggy behaviour demonstrated

The video attachment failed the first time, so I had to reencode it for a smaller file size.

Comment 2

[BugBot [:suhaib / :marco/ :calixte]]

Bugbug thinks this bug should belong to this component, but please revert this change in case of error.

Comment 3

[Jaume Delclòs Coll]

I have no idea if Bugbug is right or not in setting the category to Web Apps, but somebody should probably check.

Anyway, I will explain what I have done to try to debug this:

Enable remote debugging over USB
Hook to the Main Process (not the tab)
See that when the bug happens, an error is printed to the console:

Failed to install: messageManager is null browser.js:formatted:1910
    installManifest browser.js:1924
    AsyncFunctionThrow self-hosted:843

Look around the code to find that the manifest object (the one that fails to install) has a property _browser where messageManager is null.

I suspect this _browser object refers to the old tab and is invalid. If getManifest in Manifest.jsm got patched the following way, I think the bug would go away, but I'm not familiar enough with the code to say for sure:

    if (manifestUrl in this.manifestObjs) {
+     this.manifestObjs[manifestUrl]._browser = browser;
      return this.manifestObjs[manifestUrl];
    }

I have recorded a very short video that shows this in the debugger: https://www.youtube.com/watch?v=Ff2nTuFoVU0

Comment 4

[Jaume Delclòs Coll]

Attachment: Video demonstrating the bug in the debugger

I have added the second video as an attachment too.

Comment 5

[Diana Rus]

Hi,
I checked with a Google Pixel 3a XL and Sony Xperia Z 5 (Android 7.0) on Firefox Release 68.1.1 and Firefox Beta 68.2b6 versions from playstore and reproduced the issue.

Note:

• Through Fenix/Firefox Preview this option does not seem to be present yet based on ticket: https://github.com/mozilla-mobile/fenix/issues/204 .

Comment 6

[Jaume Delclòs Coll]

Attachment: a2hs_1587793.patch

Phabricator seems hard to set up, so for now I am attaching the patch I made with mercurial.
I assume _ in "_browser" stands for private and thus modifying the property from outside like this is bad style, but this one line change fixes the bug.

Comment 7

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

James, do you think we should accept this patch? And if so, can you nominate a reviewer?

Comment 8

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

Jaume, thank you very much for the thorough investigation and the patch. Fennec is in maintenance mode, but I think this is a trivial fix with high impact so I will find out how we can land this.

Comment 9

[James Willcox (:snorp) ([e-mail]) (he/him)]

Seems like it might be ok to me? I think Marcos maintains this code.

Comment 10

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

Marcos would you mind helping out with this bug? Do you think this is low risk?

Comment 11

[Marcos Caceres [:marcosc]]

Yes, I can take a look.

Comment 12

[Marcos Caceres [:marcosc]]

At some point in the future, we should probably switch the cache to be a WeakMap keyed off the browser objects. I'm worried that the current code keeps copies of browser objects, which are quite expensive. It would probably also catch this kind of bug too.

Comment 13

[Marcos Caceres [:marcosc]]

Ok, yeah... I'm looking at this more and I think we need to refactor all this code... I'll send a patch that should at least address this, but then we need to switch all this to not hold strong references to browser object. My concern is that if one can spin up multiple tabs (3x) each will override the associated browser for a cached manifest (leading to confusion or worst).

Comment 14

[Marcos Caceres [:marcosc]]

Attachment: re-associate manifest with a browser when getting them from cache

Addresses the problem of cached manifests having the wrong associated browser.
However, this is only temporary solution, as cached manifests can have different
associated browsers (e.g., if 3x more tabs are open, this will still potentially be racy).

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ef633bc5dc59
re-associate manifest with a browser when getting them from cache r=snorp

Comment 16

[Stefan Hindli [:stefan_hindli]]

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

Comment 17

[Sorina Florean [:sflorean]]

Marcos, can you please add a test build in order to verify the fix before will be landed on esr68? Thanks!

Comment 18

[Marcos Caceres [:marcosc]]

Spun up try run, including Android 🤞:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ab4e8a621c8f68b40de6edbda5c502d7565856db

Comment 19

[Marcos Caceres [:marcosc]]

Hi Sorina, tests seem ok but please let me know if I've run all the right things above.

Comment 20

[Marcos Caceres [:marcosc]]

Just a quick shout out to Jaume Delclòs Coll for figuring out where the bug was! Save us a bunch of time figuring this out.

Jaume, if you get a chance to apply the patch, would like to hear if it works ok for you.

Comment 21

[Jaume Delclòs Coll]

Hi Marcos, since fennec is not in mozilla-central anymore, I have applied the patch on top of 55c8e3ba286f (tip of mozilla-esr68 right now) and it seems to work perfectly as far as I have tested.

Comment 22

[Marcos Caceres [:marcosc]]

That's great news, Juame. Thanks so much for checking! Really appreciate it.

Comment 23

[Marcos Caceres [:marcosc]]

Ok, sent a try build ontop of ESR68:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ba2e478fed83311b8e4831939cbe371cf605fd93

Comment 24

[Laurentiu Apahidean]

I tested the build from Comment 23 with Pixel 3 XL (Android 9) and the issue no longer occurs.

Comment 25

[Marcos Caceres [:marcosc]]

Thank you Laurentiu!

I'm unsure now how to "request uplift" to ESR68. Bouncing back to Sorina for guidance.

Comment 26

[Marcos Caceres [:marcosc]]

Comment on attachment 9102049 [details]
re-associate manifest with a browser when getting them from cache

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: The bug affects users who have installed PWAs.
• User impact if declined: Yes - see the bug for details.
• Fix Landed on Version: 72
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The patch has been QE verified as working as expected. We also checked it on try on the relevant Android builds.
• String or UUID changes made by this patch: none, I think?

Comment 27

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9102049 [details]
re-associate manifest with a browser when getting them from cache

Fixes a Fennec PWA regression. Approved for Fennec 68.3b3.

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr68/rev/ad36747020f6

Comment 29

[Diana Rus]

Hi, verified as fixed with Google Pixel 3 XL (Android 9), Samsung Galaxy S9 (Android 8.0.0) and Sony Xperia Z5 (Android 7.0) on Firefox Beta 68.3b3.
Will leave the ticket with status unchanged until the fix reaches the release version.

Comment 30

[Laurentiu Apahidean]

I have tested the issue on 68.3.0 RC using a Samsung Galaxy S9 (Android 8.0.0) and the issue no longer occurs. I will mark it accordingly.