1874114 - Ensure any exceptions or unexpected data doesnt prevent restoring of all the other tabs in a session

Status: NEW

(Firefox :: Session Restore, defect, P2)

Description

[Kestrel]

Attachment: Session restore only shows blank New Tab

STR:

1. Launch Nightly 123.0a1 (2024-01-01) and visit about:firefoxview-next in a normal tab.
2. Duplicate the tab.
3. Update to latest Nightly 123.0a1 (2024-01-10).
4. Restore previous session.

Shows a blank page with title "New Tab" instead of restoring previous tabs. Fails trying to create about:firefoxview-next which blocks session restore and causes it to be overwritten.

Browser Console error:

NS_ERROR_ILLEGAL_VALUE: Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsIAboutModule.getURIFlags] E10SUtils.sys.mjs:435

getRemoteTypeForURIObject (resource://gre/modules/E10SUtils.sys.mjs#428)
getRemoteTypeForURI (resource://gre/modules/E10SUtils.sys.mjs#384)
createTabsForSessionRestore (chrome://browser/content/tabbrowser.js#3167)
ssi_restoreWindow (resource:///modules/sessionstore/SessionStore.sys.mjs#5071)
_restoreWindowsFeaturesAndTabs (resource:///modules/sessionstore/SessionStore.sys.mjs#5245)
_restoreWindowsInReversedZOrder (resource:///modules/sessionstore/SessionStore.sys.mjs#5269)
ssi_restoreLastSession (resource:///modules/sessionstore/SessionStore.sys.mjs#4516)
ssi_restoreLastSession (resource:///modules/sessionstore/SessionStore.sys.mjs#4515)
ss_restoreLastSession (resource:///modules/sessionstore/SessionStore.sys.mjs#654)

Regression window:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=3f6550f8444c7b29d80351648aaa682f92473d34&tochange=c2654b23e1c4a78dec94da45dd8fa1c9df31456a

Regressed by Bug 1864534.

Comment 1

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

:nsharpley, since you are the author of the regressor, bug 1864534, could you take a look? Also, could you set the severity field?

For more information, please visit BugBot documentation.

Comment 2

[Sarah Clements [:sclements]]

I wasn't able to reproduce this on latest Nightly. There have been issues with session restore when updating software, so this doesn't seem related. Sam, were you able to reproduce this?

Comment 3

[Sam Foster [:sfoster] (he/him)]

STR:

1. Launch Nightly 123.0a1 (2024-01-01) and visit about:firefoxview-next in a normal tab.

Bug 1864534 removes the about:firefoxview-next redirector, so I think the question is rather why do we not get the "Hmm. That address doesn’t look right." page. That is a separate problem though which I'll file in the firefoxview component.

NS_ERROR_ILLEGAL_VALUE: Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsIAboutModule.getURIFlags] E10SUtils.sys.mjs:435

getRemoteTypeForURIObject (resource://gre/modules/E10SUtils.sys.mjs#428)
getRemoteTypeForURI (resource://gre/modules/E10SUtils.sys.mjs#384)
createTabsForSessionRestore (chrome://browser/content/tabbrowser.js#3167)
ssi_restoreWindow (resource:///modules/sessionstore/SessionStore.sys.mjs#5071)
_restoreWindowsFeaturesAndTabs (resource:///modules/sessionstore/SessionStore.sys.mjs#5245)
_restoreWindowsInReversedZOrder (resource:///modules/sessionstore/SessionStore.sys.mjs#5269)
ssi_restoreLastSession (resource:///modules/sessionstore/SessionStore.sys.mjs#4516)
ssi_restoreLastSession (resource:///modules/sessionstore/SessionStore.sys.mjs#4515)
ss_restoreLastSession (resource:///modules/sessionstore/SessionStore.sys.mjs#654)

We've had similar errors like this before. While the exact STR and regression aren't valid, I think the symptoms are something we could work on - encountering something unexpected shouldn't prevent session restore, we should just skip that tab (and ideally, not save it in the first place.) I'm adjusting the keywords and title accordingly. P2 due to potential for dataloss.

Comment 4

[Sam Foster [:sfoster] (he/him)]

To confirm, I can reproduce this.

• Using build prior to bug 1864534, open several tabs, including one at "about:firefoxview-next".
• Check the "Open previous windows and tabs" option in about:preferences
• Close the browser
• Update to latest nightly (Note this may get fixed by bug 1874253, so you'll need a build from before any patch on that bug lands.)

ER:

• All the previously open tabs should be restored. As "about:firefoxview-next" is no longer a valid about: URL, we'd expect a "That address doesn’t look right" error page (covered by 1874253), or, failing that it just loads about:blank.
  AR:
• Tabs do not get restored.
• An exception is logged: JavaScript error: resource://gre/modules/E10SUtils.sys.mjs, line 435: NS_ERROR_ILLEGAL_VALUE: Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsIAboutModule.getURIFlags]

That exception comes from E10SUtils.sys.mjs, the stack trace looks like:

getRemoteTypeForURIObject@resource://gre/modules/E10SUtils.sys.mjs:435:26
getRemoteTypeForURI@resource://gre/modules/E10SUtils.sys.mjs:384:17
createTabsForSessionRestore@chrome://browser/content/tabbrowser.js:3167:47
ssi_restoreWindow@resource:///modules/sessionstore/SessionStore.sys.mjs:5071:29
_restoreWindowsFeaturesAndTabs@resource:///modules/sessionstore/SessionStore.sys.mjs:5245:12
_restoreWindowsInReversedZOrder@resource:///modules/sessionstore/SessionStore.sys.mjs:5269:10
ssi_restoreWindows/<@resource:///modules/sessionstore/SessionStore.sys.mjs:5340:12

createTabsForSessionRestore builds a document fragment with all of the tabs to be restored. When the call to E10SUtils.getRemoteTypeForURI throws an exception, we bail out of the loop and restore none of the tabs.

This particular case with the partly-removed about: page has a root cause we can fix elsewhere, but it seems likely there are other spots in this same block where unexpected tab data might result in an exception, and we should be able to catch those and continue restoring all the other tabs.

Comment 5

[Sam Foster [:sfoster] (he/him)]

I'm trying to figure out if this is a potentially wider problem than the exact STR and issue with about:firefoxview-next. Do you know what kind of input to getRemoteTypeForURI might result in an uncaught exception :Gijs? Or if there are other bugs or ways for me to find out?

Comment 6

[:Gijs (he/him)]

(In reply to Sam Foster [:sfoster] (he/him) from comment #5)

I'm trying to figure out if this is a potentially wider problem than the exact STR and issue with about:firefoxview-next. Do you know what kind of input to getRemoteTypeForURI might result in an uncaught exception :Gijs? Or if there are other bugs or ways for me to find out?

No, I don't have any other context - I think this is the first time I've seen this particular method throw an exception.

Thinking about what might cause that, the first obvious thing is that getRemoteTypeForURI just takes a string for the "URI", not a "real" URI object, so parsing discrepancies and/or failures could cause exceptions. (This is a pet peeve of mine - ideally the frontend should use URI objects as much as possible to avoid differing "fixup" flags etc. causing bugs when we finally do want a "real" URI.)

... but fixup exceptions appear to be caught and will return the default remote type.

So the next thing to do would be to go through getRemoteTypeForURIObject and check whether things it calls can throw. Based on this bug, clearly trying to get about URI flags for an unrecognized about URL can throw. But there are a bunch of other XPCOM calls in the call graph from the getRemoteTypeForURIObject call that can theoretically throw, though probably/hopefully mostly for esoteric URIs...

The typical question for something like this is... where to catch the exception? AIUI the E10SUtils method is intended to select the right process early - it's a perf optimization in many respects. We have safeguards in place that will stop (or switch process for) a load later on if we "guessed" wrong about what process to use. From that PoV, having it be failsafe and guarantee at least "some" answer rather than an exception, seems the most reasonable. (ni Nika to confirm I'm right about this and there's not some intent to shift more of this responsibility into this method)

So I'd suggest wrapping things inside getRemoteTypeForURIObject into a try...catch for cases that can reasonably throw.

Separately/additionally, createTabsForSessionRestore should probably be resilient in a way where restoring 1 tab where something breaks, doesn't break restoring all the other ones. That could be fixed in session restore.

Comment 7

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

(In reply to :Gijs (he/him) from comment #6)

So the next thing to do would be to go through getRemoteTypeForURIObject and check whether things it calls can throw. Based on this bug, clearly trying to get about URI flags for an unrecognized about URL can throw. But there are a bunch of other XPCOM calls in the call graph from the getRemoteTypeForURIObject call that can theoretically throw, though probably/hopefully mostly for esoteric URIs...

FWIW the reason why the call threw for about:firefoxview-next is because while that about module was removed from the about redirector, it is still registered with the static component manager (https://searchfox.org/mozilla-central/rev/f465027ef4d334dbc9ad270718c8a5e8045e7a2b/browser/components/about/components.conf#17), so the code still thinks the about module exists, meaning the check from getAboutModule (https://searchfox.org/mozilla-central/rev/f465027ef4d334dbc9ad270718c8a5e8045e7a2b/toolkit/modules/E10SUtils.sys.mjs#428) returned a module when it shouldn't have.

I think it is unexpected for a properly specified about module to throw when accessing flags, which is probably why we haven't found the need to add a try/catch around this in the past, like we have in other fallible places in this logic. We should probably still make this more resilient to exceptions from getURIFlags, but those exceptions are unexpected.

A quick scan also noticed mustLoadURLRemotely, though that explicitly ignores errors due to missing packages (https://searchfox.org/mozilla-central/rev/f465027ef4d334dbc9ad270718c8a5e8045e7a2b/chrome/nsChromeRegistry.cpp#380-385), so seems quite unlikely to actually throw.

The typical question for something like this is... where to catch the exception? AIUI the E10SUtils method is intended to select the right process early - it's a perf optimization in many respects. We have safeguards in place that will stop (or switch process for) a load later on if we "guessed" wrong about what process to use. From that PoV, having it be failsafe and guarantee at least "some" answer rather than an exception, seems the most reasonable. (ni Nika to confirm I'm right about this and there's not some intent to shift more of this responsibility into this method)

This is correct. The logic in getRemoteTypeForURIObject returning an incorrect answer is OK, and should just lead to unnecessary extra process switches. Previously, it was used for process selection of remote workers, but that was changed in bug 1850589. Overall I hope to generally move more responsibility away from this method rather than increasing the responsibility of the method.

So I'd suggest wrapping things inside getRemoteTypeForURIObject into a try...catch for cases that can reasonably throw.

This is probably reasonable, we could probably recover in all cases, log the error out somehow, and return DEFAULT_REMOTE_TYPE in those error cases. The biggest risks of overbroad try/catch calls is probably that we make a change causing unexpected failures, and end up process switching unnecessarily, or that an error page ends up loading in a different process than it otherwise would.

Separately/additionally, createTabsForSessionRestore should probably be resilient in a way where restoring 1 tab where something breaks, doesn't break restoring all the other ones. That could be fixed in session restore.

I expect there are other callers of getRemoteTypeForURI which could be hit and cause issues as well, so fixing it more centrally is probably preferable.

Comment 8

[Nikki Sharpley (:nikkis) (she/her)]

FWIW the reason why the call threw for about:firefoxview-next is because while that about module was removed from the about redirector, it is still registered with the static component manager (https://searchfox.org/mozilla-central/rev/f465027ef4d334dbc9ad270718c8a5e8045e7a2b/browser/components/about/components.conf#17), so the code still thinks the about module exists, meaning the check from getAboutModule (https://searchfox.org/mozilla-central/rev/f465027ef4d334dbc9ad270718c8a5e8045e7a2b/toolkit/modules/E10SUtils.sys.mjs#428) returned a module when it shouldn't have.

I have a patch (https://phabricator.services.mozilla.com/D196347) landing today that removes firefoxview-next from the static component manager that should at least resolve this portion of the bug.

Comment 10

[Sam Foster [:sfoster] (he/him)]

The specific cause for this issue was fixed in bug 1874253. I'm going to morph this bug and update the summary as we'd like to make session restore more robust to unexpected input like this. If a tab is unable to be restored and throws some exception, that shouldn't prevent the restoration of all the other tabs.

Comment 11

[Jeff Muizelaar [:jrmuizel]]

This bug also seems to have caused updates to break for me (bug 1875502)