1630993 - Incorrect remoteness on lazy tab browsers upon session restoring a non-remote tab

Status: NEW

(Firefox :: Session Restore, defect, P2)

Description

[Rob Wu [:robwu]]

STR:

1. Start Firefox, open a new window, open two tabs, e.g. about:preferences and about:config.
2. In about:config, set devtools.chrome.enabled to true (to enable the console for later).
3. Close the window.
4. Restore the closed window: Press Alt to show the menu, > History > Restore Closed Windows > "Advanced Preferences (and 1 other tab) (Ctrl-Shift-N)"
5. Open the global JavaScript console (Ctrl-Shift-J)
6. Run the following code: gBrowser.browsers[0].getAttribute("remote")
7. Right-click on the first tab of the window (from step 4) > Move Tab > Move to New Window

Expected:

• At step 6, the output should be "false".
• At step 7, the tab should be moved to a new window.

Actual:

• At step 6, the output is "true", which means that the lazy browser has the incorrect remoteness.
• At step 7, a new window is opened but has a blank tab instead of the tab-to-be-moved. The tab is no longer visible in the original window, but it has not properly been removed (e.g. it still appears in gBrowser.browsers, and upon closing the tab there is a prompt to ask whether the window with two tabs should be closed).

Extra details:

• Step 6 fails because the remoteness in addTab is based on aURI, but that is replaced with about:blank for lazy browsers, so remoteType ends up being "web" (DEFAULT_REMOTE_TYPE) instead of null (NO_REMOTE).
• Step 7 fails because the tab browser silently fails to swap when the remoteness does not match.

PS. Step 6's stack trace is as follows (Nightly 76.0a1 buildID 20200325093457 on Linux):

addTab@chrome://browser/content/tabbrowser.js:2715:26
addTrustedTab@chrome://browser/content/tabbrowser.js:2523:19
addMultipleTabs@chrome://browser/content/tabbrowser.js:2951:22
ssi_restoreWindow@resource:///modules/sessionstore/SessionStore.jsm:4554:27
_restoreWindowsFeaturesAndTabs@resource:///modules/sessionstore/SessionStore.jsm:4703:12
_restoreWindowsInReversedZOrder@resource:///modules/sessionstore/SessionStore.jsm:4727:10
ssi_restoreWindows/<@resource:///modules/sessionstore/SessionStore.jsm:4787:12
promise callback*ssi_restoreWindows@resource:///modules/sessionstore/SessionStore.jsm:4781:29
ssi_undoCloseWindow/<@resource:///modules/sessionstore/SessionStore.jsm:3536:12
promise callback*ssi_undoCloseWindow@resource:///modules/sessionstore/SessionStore.jsm:3535:49
ss_undoCloseWindow@resource:///modules/sessionstore/SessionStore.jsm:355:33
undoCloseWindow@chrome://browser/content/browser.js:8173:27
oncommand@chrome://browser/content/browser.xhtml:1:1

Comment 1

[Mike de Boer [:mikedeboer]]

Ah, thanks for the find, Rob!

So it looks like the following needs to be done here to fix it, and it's all in tabbrowser.js:

• The code starting at line here, we'll need to change to

      let URIToLoad = aURI = aURI || "about:blank";
      let URIObject = null;
      try {
        URIObject = Services.io.newURI(URIToLoad);
      } catch (ex) {
        /* we'll try to fix up this URL later */
      }

      let lazyBrowserURI;
      if (createLazyBrowser && URIToLoad != "about:blank") {
        lazyBrowserURI = aURIObject;
        URIToLoad = "about:blank";
      }

      var uriIsAboutBlank = URIToLoad == "about:blank";

• Bonus points for changing aURIObject to URIObject
• Change all occurrences of aURI in this method to URIToLoad, like in the code sample above. EXCEPT line 2676, lines 2794 'till 2796! Add a comment there explaining why we're doing that. (e.g. "We're using aURI here to make sure that we're checking the value as it was passed in.")

That should make it work properly again.

Comment 3

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

The severity field is not set for this bug.
:mikedeboer, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 4

[ariasuni]

Trying to move a discarded tab to an existing window through the WebExtensions API now simply remove it from the source window, from an external point of view; while before it «changed» it to an about:blank.

Comment 5

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

Redirect a needinfo that is pending on an inactive user to the triage owner.
:dao, since the bug has high priority, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 6

[Vitalii]

Re-checked bug 1595583, it no longer reproduces.
Bug 1595583 depends on 1630993.
Perhaps bug 1595583 also no longer replicates.

Comment 7

[Vitalii]

(In reply to Vitaliy from comment #6)

Re-checked bug 1595583, it no longer reproduces.
Bug 1595583 depends on 1630993.
Perhaps bug 1630993 also no longer replicates.