1641715 - GeckoView doesn't load for external apps (custom tabs/PWAs) in AC and Fenix

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, defect, P1)

Description

[Tiger Oakes]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101 Firefox/78.0

Steps to reproduce:

For about a month in Fenix, opening a PWA or custom tab sometimes gets a white screen rather than the webpage loading. This also occurs in Sample Browser in Android Components. If you remove the main app from memory by clearing recent apps, PWAs will not load.

I checkout out lots of old Android Components versions from late April and early May to track down where the bug appeared. (See https://github.com/mozilla-mobile/fenix/issues/10689.) I narrowed down one line: changing the nighly GeckoView version from "77.0.20200428100141" to "77.0.20200429095105".

There are not many behaviour changes between PWAs and the main browser app, so we're not sure why this issue has sprung up.

This is a high priority bug and needs to be fixed before Fenix's feature freeze.

Actual results:

White screen loads after GV version change

Expected results:

Webpage should load, just like before the change.

Comment 1

[Jonathan Almeida [:jonalmeida]]

This would be a Fenix release blocker for PWAs.

Comment 2

[Emily Toop (:fluffyemily)]

When is Fenix's feature freeze?

We can get this scheduled in for landing 79. It sounds like you would want this uplifted to 78 if it is a release blocker?

Comment 3

[Jonathan Almeida [:jonalmeida]]

If possible, yes, otherwise we would have to consider disabling the feature and we would need to get buy in from product for that.

Comment 4

[Chenxia Liu [:liuche]]

Feature freeze is 6/5, however we'll take bugfixes through the end of June. However, if we can get this into 78 beta, then we can test this in Fenix release channel.

Comment 5

[Emily Toop (:fluffyemily)]

I've just been bumped on this by :grisha. This is still a Fenix P1.

Comment 6

[Dylan Roeh (:droeh) (he/him)]

This appears to have resolved itself -- I updated my tree today and can no longer reproduce on local builds; checking nightly also looks good, and I asked Jon to try it out and he also can no longer reproduce it. (And, looking now, it appears the original reporter on the github issue posted to say it's working a few days back.)

Comment 7

[Emily Kager [:ekager]]

It looks like this has started happening again with fairly reliable STR:
https://github.com/mozilla-mobile/fenix/issues/15335

Happens on PWAs like pwa-directory.appspot.com or crypt.ee, STR:

1. Install the PWA.
2. Swipe to close Fenix.
3. Launch the PWA. -> blank page.
   After this, I get the blank screen even with Fenix in the background.

Comment 8

[Christian Sadilek [:csadilek]]

After upgrading to GV 84.0a1-20201114094625 we're seeing this a lot more now. I can reproduce every time using latest Fenix Nightly (201116 17:01).

Stepping back to GV 84.0a1-20201109095222 fixes this. So it looks like we have a recent regression?

Going through the logs we see the pageStart event, but then nothing seems to happen and we never get a pageStop or any other event. The app is responsive and nothing is blocking the main thread.

STR (similar to before):

• Launch Fenix

• Install PWA

• Close Fenix

• Open PWA (fails intermittently, but consistently using 84.0a1-20201114094625)

Dylan, Agi, can you help us investigate? Any idea what could've regressed this? We've already looked at our commits in this range and didn't find anything.
Good: GV 84.0a1-20201109095222
Bad: GV 84.0a1-20201114094625

Attaching debug logs as well.

Comment 9

[Christian Sadilek [:csadilek]]

Attachment: Debug logs while opening PWA

Comment 10

[[ex-Mozilla] Agi Sferro | :agi]

Looks like this is caused by Multi E10S (turning dom.ipc.processCount back to 1 fixes it), Aaron any ideas?

Comment 11

[(No longer employed by Mozilla) Aaron Klotz]

This is probably one of those things where some bug existed but was hard to reproduce, and then e10s-multi showed up and exacerbated it. I'll take a look...

Comment 12

[(No longer employed by Mozilla) Aaron Klotz]

One thing I've noticed when reproducing this is that the PWA load causes Gecko to launch a second content process even though the first one was sitting there unused. We also ended up with both content processes set to FOREGROUND priority, which should not happen.

Comment 13

[(No longer employed by Mozilla) Aaron Klotz]

Something is definitely happening out of order here, but I haven't narrowed it down yet. Stepping through this case with strategically-placed breakpoints produced a working instance of the PWA.

Comment 14

[(No longer employed by Mozilla) Aaron Klotz]

Just to leave a couple of notes here for when I get back:

• I have figured out why we sometimes see two content processes and other times one: There is a service worker that is being spun up. If the PWA's web content loads before the service worker, we get a second process. If the service worker loads before the web content, we only get one content process. This is because of the way ContentParent's e10s process allocator works: it tracks top-level content but not service workers. If tab0 is hosting a service worker, the allocator still considers that process to be "empty," because it's not holding any "tabs," i.e. top-level content. In that case, the allocator re-uses tab0. OTOH, if tab0 is already hosting a tab, then the allocator decides to spin up tab1 to host the service worker.
• Is this service worker business the cause? Possibly (and the blank content would be consistent with some of the issues that desktop has seen with service workers during browser startup), but more investigation is necessary.

Comment 15

[(No longer employed by Mozilla) Aaron Klotz]

At this point I think that I've taken this about as far as I can -- there are multiple possible solutions to this issue such that I think we need an assist from the DOM Service Workers peeps to get this right.

NOTE

This bug is blocking our ability to enable multi-process e10s on Android, which therefore blocks our prep work for Fission. Please prioritize accordingly.

Analysis

There are two issues at play here: The first issue here is a race between the time that the parent's PBackground thread attempts to launch a service worker, and the time that content processes have actually registered their RemoteWorkerServiceParent actors with the RemoteWorkerManager. The second issue is that the content process that eventually is expected to host the service worker never receives its permissions.

I am observing this sequence of events:

1. Content process tab0 has been started for the purposes of hosting the PWA's web content, however it is still in the process of loading;
2. The parent's main thread calls ServiceWorkerPrivateImpl::SpawnWorkerIfNeeded, which sends PRemoteWorkerControllerConstructor the parent's PBackground thread;
3. The parent's PBackground thread calls RemoteWorkerManager::Launch. At this point, content process tab0 has still not yet registered its RemoteWorkerServiceParent actor, so the RemoteWorkerManager decides to launch a new content process, tab1. It adds an entry to the RemoteWorkerManager::mPendings array.
4. tab0 finally registers its RemoteWorkerServiceParent actor. RemoteWorkerManager::RegisterActor finds a match with the pending entry added in step (3). It then invokes RemoteWorkerManager::LaunchInternal for tab0.
5. tab1 finishes launching. As part of the processLaunchCallback in RemoteWorkerManager::LaunchNewContentProcess, we call TransmitPermissionsAndBlobURLsForPrincipalInfo. At this point, tab1 has been sent permissions, but tab0 has not!
6. tab0's service worker initialization gets as far as PermissionManager::WhenPermissionsAvailable, but gets stuck there because this process never receives the service worker's permissions and thus the promises never resolve!

In conclusion, we end up with one content process that has a worker but never receives any permissions for it, and a second content process that receives permissions but never hosts the worker.

Jens, Andrew, could we get some help with this, please?

Comment 16

[Andrew Sutherland [:asuth] (he/him)]

Thank you for the excellent investigation, Aaron!

I understand there to be two things going on here:

1. There are synchronous assumptions backed into the asynchronous process of remote worker launching which leads to a hole where permissions may not be transmitted to the process that ends up being selected for a worker.
   • Permissions, or the intent to transmit permissions, happens at RemoteWorkerManager::Launch time via SelectTargetActorInternal or LaunchNewContentProcess. But the process that is selected by RemoteWorkerManager::RegisterActor may not be the one that LaunchNewContentProcess picked, and we only call RemoteWorkerManager::LaunchInternal which does not transmit permissions.
2. RemoteWorkerManager makes decisions to launch processes from the PBackground thread based on its perception of what processes are available based on the RemoteWorkerServiceParent registrations, which is very asynchronously delayed from the intent to spawn a process and can potentially lead to redundant process spawns.

I'll plan to address the first point in this bug in the short term (this week). If you know of any existing tests that make it easier to write an automated regression test for this or developed parts of one during your investigation, I'd really appreciate being pointed at them! Thanks!

For the 2nd point, I already have some WIP patches from a previous investigation with :nika that attempt to correct the sketchiness in process selection management by the RemoteWorkerManager. Specifically, it moves all process selection determinations to the main thread and explicitly results in each worker being tracked by and known to the ContentParents with explicit keep-alive objects (which are exposed to about:processes). This should avoid the redundant process spawning. This should still be able to land in the imminent 86 nightly cycle, but is more involved and potentially needs a bunch of sophisticated new tests written.

Comment 17

[Andrew Sutherland [:asuth] (he/him)]

Attachment: Bug 1641715 - Normalize permission transmission. r=rpl

Prior to this patch, permissions would be transmitted when either:

1. The choice was made to use an existing process.
2. A new process was launched to be the home for a worker.

This seems like it covers all the bases but the actual mechanism by
which a remote worker would be launched for a new process is when its
RemoteWorkerServiceParent actor is registered. The above would be
sufficient if the only way for processes to come into existence was at
the request of the RemoteWorkerService, but obviously this is not the
case. When that happened, it would be possible for a remote worker to
be placed in a new process that was not the process we were
transmitting permissions for.

The changes in this patch normalize permission transmission so that
they are transmitted at the same time we attempt to spawn the worker
via the selected RemoteWorkerServiceParent. This code path is used for
both an existing process and when a new process is spawned so there's
no longer any gap.

Comment 18

[(No longer employed by Mozilla) Aaron Klotz]

I tested Andrew's patch with the original STR, and it does indeed fix the hung PWA issue. Unfortunately the process that loses the race (tab1 from my analysis in comment 15) is left without having any content to host, but is stuck with foreground priority. This is bad for us on Android; we need any "empty" content processes to run at low priority so that Android prefers to reclaim those processes ahead of the ones that are actively hosting content.

We discussed this on Matrix. I'll leave it to Andrew to decide the way forward here.

Comment 19

[(No longer employed by Mozilla) Aaron Klotz]

I can confirm that augmenting the patch by modifying this call to prefer a used content process was sufficient to resolve this issue: we don't even bother starting tab1 in that case.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/88adf1302b6f
Normalize permission transmission. r=rpl

Comment 21

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/88adf1302b6f