1695326 - [SHIP] write a test for "about:config restores with incorrect security info"

Status: NEW

(Firefox :: Session Restore, task, P3)

Description

[Andrew McCreight [:mccr8]]

STR:

1. Open a new tab and load about:robots in it.
2. Make sure the browser is set to "restore previous session".
3. Restart the browser.

Expected behavior: the icon next to the URL in the address bar should be a piece of paper, and when you click on it, it should say "this page is stored on your computer".

Actual behavior: when session history in parent is enabled, the page restores with the magnifying glass icon, as if the tab held about:newtab.

(I was looking at this because the test that has been disabled as part of bug 1690134 uses about:robots. I don't know if it is actually related.)

Comment 1

[Andrew McCreight [:mccr8]]

Note that about:robots seems to load in the parent process.

This behavior also seemed to reproduce with SHIP enabled and Fission disabled, if that's a sensible configuration to even run.

Comment 2

[Chris Peterson [:cpeterson]]

M8 unless this affects real sites.

Comment 3

[Andrew McCreight [:mccr8]]

Sorry, I should have specified initially that the issue is with the site security info. My talk about icons made it sound like it is related to the favicons.

Comment 4

[Andrew McCreight [:mccr8]]

One thing I noticed when restoring about:robots is that I get this message in the log:
updateSessionStoreFromTablistener() with sHistoryNeeded, but no sessionHistory

I also added some logging to SessionStoreUtils.restoreDocShellCapabilities(), because Nika pointed out that there's some code in SessionStore._restoreTabState() that calls that in the same process case after it sends an async message, which seems potentially fishy. The value of data.tabData.disallow appears to be "undefined". I don't know if that is expected or not.

Comment 5

[Andrew McCreight [:mccr8]]

As you might expect, about:config exhibits the same issue.

Comment 6

[Andrew McCreight [:mccr8]]

If I comment out the same process block (starting with "For the non-remote case, the above will restore, but asynchronously.") then the error messages go away but the behavior is still incorrect, so I guess that's a red herring.

Comment 7

[u608768]

Alternate STR (avoids having to restart the browser each time):

1. Open about:robots
2. Close the tab
3. Reopen the tab with Ctrl+Shift+T

You should notice the same behavior as in comment #0.

Comment 8

[Andrew McCreight [:mccr8]]

I've been looking at this a little, but if you want to look into this, Kashav, just let me know.

Comment 9

[Andrew McCreight [:mccr8]]

I've found one difference in behavior between a fresh load of the page. browser-siteIdentity.js checks gBrowser.securityUI.isSecureContext as part of _isPotentiallyTrustworthy(), which is used to mark something as a "local resource". In a fresh load, isSecureContext is false. When we do a session restore, it is false. It is false even when about:newtab is getting passed to updateIdentity right before about:robots.

I'm not sure what is setting it to true in the fresh load case. I don't see WindowGlobalChild::OnNewDocument() getting called in either case, which is where I thought it was getting set.

I also noticed today that when it is doing the tab restore using Kashav's steps in comment 7 (ctrl+shift+t), the insecure lock icon briefly flashes in the URL bar before being replaced with the magnifying glass.

Comment 10

[Andrew McCreight [:mccr8]]

It has to be more than the isSecureContext thing, though. I edited _isSecureContext to be always true in browser-siteIdentity.js and the only noticeable impact was that instead of an insecure lock icon flashing before the magnifying glass, it is the "local file" icon flashing briefly before the magnifying glass.

Comment 11

[Andrew McCreight [:mccr8]]

Here's something else that seems interesting:

1. With Fission enabled, open about:robots in a few tabs.
2. Go into about:config and disable Fission.
3. Restart the browser and restore the about:robot tabs.

Even though Fission is disabled, the about:robot tabs have the same incorrect restore behavior. This persists through multiple restarts with Fission disabled. So maybe the issue is with how the session store is saved rather than how it is loaded?

Comment 12

[u608768]

This was interesting to debug, I think understand what's going on.

I diff'd the browser.xhtml source for about:robots from before and after restoring and the only relevant difference was the "pageproxystate" attribute: "valid" pre-restore and "invalid" post-restore. That attribute is set in UrlbarInput.setPageProxyState(). The relevant caller for us is UrlbarInput.setURI().

The function checks if the browser has a "user typed value", and if so, only sets "pageproxystate" to "valid" if the URL is for an empty or initial page. In the final call to setURI (it's called multiple times for a single load/restore), when SHIP is enabled, value is about:robots and when SHIP is disabled, it's null (about:robots is not an empty/initial page, so "pageproxystate" is set to "invalid" for SHIP, which results in us hiding the "site information" icon).

So now I tried figuring out why we're only setting userTypedValue for SHIP. It turns out we also set it for non-SHIP, but there are some ordering differences that affect when that value is cleared.

For SHIP restores:

• We start the restore and queue a "SessionStore:restoreDocShellState" message to be sent to ContentSessionStore. Because this is a parent process document, everything here is happening synchronously.
• We manage to arrive in SessionStore._restoreTabContent() before ContentSessionStore receives the message. Here we reload the current history entry.
• ContentSessionStore receives the "SessionStore:restoreDocShellState" and sets the docshell's current URI in ContentSessionStore.restoreDocShellState().
• This triggers an onLocationChange, where the tabbrowser will clear its browser's userTypedValue.
• ContentSessionStore then sends a "restoreHistoryComplete" message back to SessionStore, and now we set userTypedValue back to "about:robots".
• The browser element receives that same onLocationChange event and calls setURI.

For non-SHIP restores:

• We start the restore and set the current URI in ContentRestore.restoreHistory().
• We get that same onLocationChange event in tabbrowser, but we don't clear the userTypedValue yet. I'm still a little confused about why, but the difference is that didStartLoadSinceLastUserTyping() is false for non-SHIP. It's only flipped after the reloadCurrentEntry() call in ContentRestore. FWIW, I don't think this is actually relevant to the bug, since we clear the typed value later anyways.
• ContentSessionStore sends up the "restoreHistoryComplete" message and SessionStore sets userTypedValue to "about:restore"
• It is at this point that ContentRestore calls reloadCurrentEntry().
• This triggers an onLocationChange, and the tabbrowser clears its userTypedValue. We eventually call setURI and set pageproxystate.

So I guess the problem here is that we're relying on some message ordering that doesn't actually hold for parent process document restores.

Comment 13

[u608768]

One solution that fixes this is to just stop using ContentSessionStore for parent-process document restores. Half of the "restore docshell state" step already happens outside of ContentSessionStore, so it shouldn't be a problem to just move it all here.

Comment 14

[u608768]

This will be fixed by bug 1702055. Keeping this open so we can verify. This should probably also have a test.

Comment 15

[u608768]

Can confirm this works with the bug 1702055 patches in the 20210504092024 Nightly.

This should have a test, but I think writing that can be Fission:Future.

Comment 16

[Chris Peterson [:cpeterson]]

Clearing assignee because Kashav is no longer at Mozilla.