1673763 - [gv-junit-fis] Error page triggers `about:blank` onLocationChange with Fission

Status: RESOLVED FIXED

(GeckoView Graveyard :: Sandboxing, defect, P1)

Description

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

To reproduce this run NavigationDelegateTest#loadFileNotFound with fission enabled.

Comment 1

[Chris Peterson [:cpeterson]]

Tracking this bug for Android Fission milestone M2 (pass tests with Fission enabled).

Comment 2

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

Nika thinks this is a problem with the process switch happening, error pages don't expect to load anything different than about:neterror while we use either a resource:// or a data URI which might trigger an unexpected process switch. We will have to work with platform to make this work for Android somehow.

Comment 3

[Nika Layzell [:nika] (ni? for response)]

I'm not sure this is a good approach long term, but we may be able to check if this is the cause by hacking away the process switch when loading error pages.

To do that, we'd add an extra case to IsolationOptionsForNavigation which takes into account the mLoadStateLoadType when calculating behavior, probably something like this around here.

#ifdef MOZ_WIDGET_ANDROID
  // If we're loading an error page on android, it must complete within the same process as the
  // errored page load would complete in due to code expecting that behavior. See bug 1673763.
  if (aLoadStateLoadType == LOAD_ERROR_PAGE) {
    // XXX: Validate that this URI is OK to load as an error page somehow? It will bypass site isolation.
    // Log an error and return `Err(NS_ERROR_DOM_SECURITY_ERR)` or similar to abort and reject the load if it isn't valid.
    // Should probably at least require it to be bundled into the browser (i.e. `resource://` URI?)?

    MOZ_LOG(gProcessIsolationLog, LogLevel::Verbose,
            ("Forcing error page load to complete in the current process"));
    behavior = IsolationBehavior::Anywhere;
  }
#endif

Comment 4

[Chris Peterson [:cpeterson]]

Moving Android Fission bugs to the new GeckoView::Sandboxing component.

Comment 5

[[:owlish] 🦉 PST]

The tests affected by this:

• NavigationDelegateTest#loadFileNotFound
• NavigationDelegateTest#loadUnknownHost
• NavigationDelegateTest#loadExternalDenied
• NavigationDelegateTest#loadUntrusted
• NavigationDelegateTest#loadDeprecatedTls
• NavigationDelegateTest#safebrowsingPhishing
• NavigationDelegateTest#safebrowsingMalware
• NavigationDelegateTest#safebrowsingUnwanted
• NavigationDelegateTest#safebrowsingHarmful

Comment 7

[[:owlish] 🦉 PST]

Attachment: WIP: Bug 1673763 - WIP - change error page loading

Comment 8

[[:owlish] 🦉 PST]

Attachment: Bug 1673763 - Do not switch processes when loading error pages on Android

Comment 9

[[:owlish] 🦉 PST]

We investigated the issue and discovered that on Fission, when we load an error page on Android, we have a process switch which leads to about:blank load instead of the error page. It's important to note that on Android, we don't have standard about: error pages like on desktop, the app is expected to provide their own custom error pages.

One solution is to just not have a process switch when we load an error page on Android. It seems simple but less secure (although secure enough, as the error page URL come from the app itself via API).

Another option is to load the error pages in parent process. We would achieve that by loading a special about: address, and then in the AboutRedirector intercept that, call the GV API to ask the app for the URI, and initialize the page with that URI (the sketch of this solution can be found in the abandoned WIP patch for this bug). Because we get the URI from GV asynchronously, we need to do the redirect also asynchronously. For that, we will need to write a custom channel. It is a lot of work 🤔 But it is a more solid and more secure solution.

For now, we decided to solve the problem in a simple way to move on to the rest of the bugs because at the moment we don't know what we will discover there. After we fixed the tests, it would be nice to revisit this and add the custom channel. I will create a separate bug for that work as a clone of this one.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6fa80c1f2f66
Do not switch processes when loading error pages on Android r=geckoview-reviewers,nika,jonalmeida

Comment 11

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/6fa80c1f2f66

Comment 12

[[:owlish] 🦉 PST]

Reopening to enable the rest of the tests. I was so concentrated on the one that is in the STR, that I forgot about the others - there are quite a few of the tests that were blocked by this.

Comment 13

[[:owlish] 🦉 PST]

Attachment: Bug 1673763 - Enable more tests; make sure the comments for disabled tests are accurate

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/df174956d60e
Enable more tests; make sure the comments for disabled tests are accurate r=geckoview-reviewers,boek

Comment 15

[Cosmin Sabou [:CosminS]]

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