1696815 - "pending" attribute not removed when tab is navigated while being restored; title is not correctly set

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect)

Description

[Rob Wu [:robwu]]

bug 1498432 introduced a unit test that basically opens a discarded/lazy tab browser, and then navigates it to a new URL. As part of the tabbrowser implementation, restoration of the tab starts first before the actual navigation. The existing logic expects the pending to be removed eventually, but it is still there when Fission is turned on.
I have previously explained the relation between the "pending" attribute and the non-updating of tabs at https://bugzilla.mozilla.org/show_bug.cgi?id=1511756#c7 . It is possible for the two bugs to share the same causes, but I did not bother investigating further prior to filing this bug.

To reproduce this bug, start Firefox with Fission enabled ( MOZ_FORCE_ENABLE_FISSION=1 ) and open then navigate the tab by running the following code in the devtools console of an extension page (use about:debugging to inspect an extension; any extension will do):

t = await browser.tabs.create({ url: "http://example.com/1st", discarded:true});
await browser.tabs.update(t.id, { url: "http://example.com/2nd" });

Expected: The title should be "Example Domain"
Actual: The title is example.com/2nd.
gBrowser.tabs[2].hasAttribute("pending") is unexpectedly true. (if the new tab is the 3rd tab).

To test as a developer:

1. Remove the skip-if = fission from browser_ext_tabs_discard_reversed.js in browser/components/extensions/test/browser/browser.ini (added in bug 1498432 ).

./mach test browser/components/extensions/test/browser/browser_ext_tabs_discard_reversed.js --enable-fission

If you want to inspect the state of the browser when the failure is happening, comment out the resolve() at https://hg.mozilla.org/integration/autoland/rev/9ce142e600af#l3.61 (expected: title changed, actual/wrong: title looks like a URL).

Comment 1

[Neha Kochar [:neha]]

Rob, this is a new test that got skipped for Fission in your patch. I'm hoping you can fix this so it can be re-enabled.
This should be in scope of Fission M7 milestone i.e. Fx88 (soft freeze on March 18).

Comment 2

[Rob Wu [:robwu]]

Attachment: tabs-discarded-update.zip

The observed behavior is not a recent regression (tested in 85 up until the most recent build with mozregression).

STR:

1. Start Firefox with MOZ_FORCE_ENABLE_FISSION=1, visit about:debugging and load the attached extension.
2. Click on the extension button

Expected (e.g. seen when MOZ_FORCE_ENABLE_FISSION=1 is not set):

• Title of the opened tab is "Example Domain"

Actual (since Firefox 87, when bug 1673617 got fixed):

• Title is example.com/2nd.

(before bug 1673617 got fixed, there was an infinite navigation loop between example.com and a blank tab)

Comment 3

[Rob Wu [:robwu]]

This is a pre-existing bug that I discovered when I improved test coverage. It isn't severe enough to drop my other work in an attempt to fix this within a week. I recommend to move it to a later milestone, and hope that people who are more familiar with Session Restore + tabbrowser + SHIP can take a look at this bug.

Comment 4

[Tomislav Jovanovic :zombie]

This is likely blocked on Session Restore rewrite, which is planned for Fission M7a.

Comment 5

[Neha Kochar [:neha]]

Session Store rewrite will be complete in M7 so the work that depends on it (like this one) should be do-able in M7a. :zombie, will you be able to fix this in M7a since it's an extension test?

Comment 6

[Tomislav Jovanovic :zombie]

From my understanding, this is a bug in session restore under fission, that extensions are the only way to trigger (for now, until Firefox starts discarding tabs on memory pressure).

Once the underlying cause is fixed, I expect the test should just work (as it currently does without fission). If that doesn't happen, I'd be happy to investigate, but the issue will likely be in Session Restore and not the test.

Comment 7

[Neha Kochar [:neha]]

Kashav will test and confirm if this is still broken.

Comment 8

[u608768]

This is still happening. We remove the "pending" attribute here and here. Will look into this more.

Comment 9

[Neha Kochar [:neha]]

Anny, can you please look into this more, and fix it?

Comment 10

[u608768]

Might be fixed with the patches from bug 1702055.

Edit: it is not.

Comment 11

[u608768]

The problem is that we don't have anything for the "correctly handle browser.loadURI() calls from foreign code" case for SHIP restores. Removing this didn't break anything for bug 1673617, so we didn't bother carrying it over to the SHIP codepath, but looks like it's required after all. We can use addProgressListenerForRestore with STATE_START in this function to add similar behavior to SHIP restores.

Comment 12

[Anny Gakhokidze [:annyG]]

I'm pausing work on this for a bit but here are some notes.

In the old code (as per kashav's comment), we used to add a listener to correctly handle browser.loadURI() calls from foreign code. In onStartRequest, the listener would call ContentSessionStore's restoreTabContentStarted and pass a callback (more on it later). Then, it would call another callback to notify the parent that the load started. This callback would eventually call cause SessionStoreInternal._restoreTabContentStarted to be called.

So, roughly this is the code that get's invoked (to visualize it):

this._progressListener = new ProgressListener(this.docShell, {
      onStartRequest: () => {
        // Some code called browser.loadURI() on a pending tab. It's safe to
        // assume we don't care about restoring scroll or form data.
        this._tabData = null;

        // Listen for the tab to finish loading.
        this.restoreTabContentStarted(callbacks.onLoadFinished);

        // Notify the parent.
        callbacks.onLoadStarted();
      },
    });

ContentSessionStore.restoreTabContentStarted does a few things:

• uninstalls the reload listener
• uninstalls the old progress listener (with onStartRequest callback)
• adds a new progress listener with onStopRequest callback, which just calls onLoadFinished (see below).

callbacks.onLoadStarted() is

      onLoadStarted: () => {
        // Notify the parent that the tab is no longer pending.
        this.mm.sendAsyncMessage("SessionStore:restoreTabContentStarted", {
          epoch,
        });
      },

which eventually calls SessionStoreInternal._restoreTabContentStarted.

and callbacks.onLoadFinished is

    onLoadFinished: () => {
        // Tell SessionStore.jsm that it may want to restore some more tabs,
        // since it restores a max of MAX_CONCURRENT_TAB_RESTORES at a time.
        this.mm.sendAsyncMessage("SessionStore:restoreTabContentComplete", {
          epoch,
        });
      },

which eventually calls SessionStoreInternal._restoreTabContentComplete.

So the old code roughly does the following:

• add a progress listener with onStartRequest callback
• when onStartRequest gets called, call restoreTabContentStarted
  • uninstall old listeners and install a new one with onStopRequest, which will eventually call SessionStoreInternal._restoreTabContentComplete.
• notify the parent that the load has started by sending a message which eventually causes SessionStoreInternal._restoreTabContentStarted to be called.

And again, repeating everything but with the correct ordering:

• add a progress listener with onStartRequest
• onStartRequest gets called, uninstall old listeners and install a new listener with onStopRequest
• notify the parent that the load has started (causes SessionStoreInternal._restoreTabContentStarted to be called)
• sometime later, onStopRequest in the new listener gets called, and eventually we execute code in SessionStoreInternal._restoreTabContentComplete.

As per convo with kashav in the DM, this is the idea of how events are to fold out when we are restoring a tab and loadURI gets called:

• we start a restore
• we get a STATE_START during the restore history phase (and restore tab content phase?), which signifies some foreign code attempted to load a url in the browser (that differs from the one being restored?)
• we clear our restore data, since we won't be restoring anymore, and remove listeners and other stuff associated with the restore
• when that new load finishes we still fire the "restore complete" events

Comment 13

[Anny Gakhokidze [:annyG]]

Attachment: WIP: Bug 1696815 - WIP Cleaned up patches to fix pending tab attr

Comment 14

[Anny Gakhokidze [:annyG]]

GOODBYE bug, assigning it to kashav

Comment 15

[u608768]

Attachment: Bug 1696815 - Handle browser.loadURI() calls that happen before the browser is fully restored, r?annyG,nika

Comment 16

[u608768]

Attachment: Bug 1696815 - Wait for the _restoreHistory() promise before firing SSTabRestored, r?annyG

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ca4535b8d3aa
Handle browser.loadURI() calls that happen before the browser is fully restored, r=annyG,nika,zombie
https://hg.mozilla.org/integration/autoland/rev/b52bf621dbe4
Wait for the _restoreHistory() promise before firing SSTabRestored, r=annyG

Comment 18

[Alexandru Michis [:malexandru]]

Backed out 2 changesets (Bug 1696815) for causing bc failures in browser_ext_tabs_discard_reversed.js
Backout link: https://hg.mozilla.org/integration/autoland/rev/0703a1f279b0df6871e5e252291a8fd6e0b973dd
Push with failures, failure log.

Comment 19

[u608768]

Rob, the test from comment #0 is failing intermittently here with:

FAIL changes to tab.url after update - Expected: ["http://example.com/initiallyDiscarded","http://example.com/browser/browser/components/extensions/test/browser/file_dummy.html"], Actual: ["http://example.com/browser/browser/components/extensions/test/browser/file_dummy.html"] - 

i.e., we're not seeing an onUpdated event with the tab's initial URL. It's not clear that the failure is related to the session restore changes. Are there known issues with onUpdated with Fission? I've found bug 1669356 (and other onUpdated bugs), but that wouldn't explain why we're only failing with Fission.

Comment 20

[u608768]

Some basic logging in the status listener shows some differences:

Fission:

 PASS Tab initially discarded - 
 PASS Initial URL - Expected: http://example.com/initiallyDiscarded, Actual: http://example.com/initiallyDiscarded - 
 PASS Initial title - Expected: http://example.com/initiallyDiscarded, Actual: http://example.com/initiallyDiscarded - 
 PASS tabId for tabs.onUpdated - Expected: 17, Actual: 17 - 
 Got status change with url = undefined
 Got status change with url = undefined
 Got status change with url = http://example.com/browser/browser/components/extensions/test/browser/file_dummy.html
 Got status change with url = http://example.com/browser/browser/components/extensions/test/browser/file_dummy.html
 PASS tabId for tabs.onUpdated - Expected: 17, Actual: 17 - 
 PASS tabId for tabs.onUpdated - Expected: 17, Actual: 17 - 
 PASS tabId for tabs.onUpdated - Expected: 17, Actual: 17 - 
 Got status change with url = undefined
 Got status change with url = undefined
 PASS onCompleted for tab - Expected: 17, Actual: 17 - 
 PASS URL ater load - Expected: http://example.com/browser/browser/components/extensions/test/browser/file_dummy.html, Actual: http://example.com/browser/browser/components/extensions/test/browser/file_dummy.html - 
 PASS changes to tab.discarded after update - Expected: [false], Actual: [false] - 
 FAIL changes to tab.url after update - Expected: ["http://example.com/initiallyDiscarded","http://example.com/browser/browser/components/extensions/test/browser/file_dummy.html"], Actual: ["http://example.com/browser/browser/components/extensions/test/browser/file_dummy.html"] - 

Non-Fission:

 PASS Tab initially discarded - 
 PASS Initial URL - Expected: http://example.com/initiallyDiscarded, Actual: http://example.com/initiallyDiscarded - 
 PASS Initial title - Expected: http://example.com/initiallyDiscarded, Actual: http://example.com/initiallyDiscarded - 
 PASS tabId for tabs.onUpdated - Expected: 3, Actual: 3 - 
 Got status change with url = undefined
 Got status change with url = undefined
 Got status change with url = undefined
 Got status change with url = undefined
 Got status change with url = http://example.com/initiallyDiscarded
 Got status change with url = http://example.com/initiallyDiscarded
 Got status change with url = undefined
 Got status change with url = undefined
 PASS tabId for tabs.onUpdated - Expected: 3, Actual: 3 - 
 Got status change with url = http://example.com/browser/browser/components/extensions/test/browser/file_dummy.html
 Got status change with url = http://example.com/browser/browser/components/extensions/test/browser/file_dummy.html
 PASS tabId for tabs.onUpdated - Expected: 3, Actual: 3 - 
 PASS tabId for tabs.onUpdated - Expected: 3, Actual: 3 - 
 PASS tabId for tabs.onUpdated - Expected: 3, Actual: 3 - 
 Got status change with url = undefined
 Got status change with url = undefined
 PASS onCompleted for tab - Expected: 3, Actual: 3 - 
 PASS URL ater load - Expected: http://example.com/browser/browser/components/extensions/test/browser/file_dummy.html, Actual: http://example.com/browser/browser/components/extensions/test/browser/file_dummy.html - 
 PASS changes to tab.discarded after update - Expected: [false], Actual: [false] - 
 PASS changes to tab.url after update - Expected: ["http://example.com/initiallyDiscarded","http://example.com/browser/browser/components/extensions/test/browser/file_dummy.html"], Actual: ["http://example.com/initiallyDiscarded","http://example.com/browser/browser/components/extensions/test/browser/file_dummy.html"] - 

Sounds like we're sometimes setting the URL earlier than before with Fission enabled?

Comment 21

[u608768]

It looks like we're not getting a browsing context here (https://searchfox.org/mozilla-central/rev/9975889f5c0d5c59bd22121a454beba774cbae71/dom/ipc/BrowserParent.cpp#2928), which is causing us to drop the http://example.com/initiallyDiscarded location change event.

Comment 22

[u608768]

Okay, so the test loads PAGE_URL on a discarded browser. This causes a process switch from about:blank -> example.com. It expects a location change for PAGE_URL_BEFORE, but depending on when the process switch happens we may or may not drop that event because of comment #21.

Interestingly this doesn't really fail if run once, but with --repeat n it will consistently fail in the 2nd or 3rd run. Probably because we now have a preallocated example.com process?

Comment 23

[u608768]

Not a bug in extensions, test is just not compatible with Fission.

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/648737f096f7
Handle browser.loadURI() calls that happen before the browser is fully restored, r=annyG,nika,zombie
https://hg.mozilla.org/integration/autoland/rev/28a8b1e0114f
Wait for the _restoreHistory() promise before firing SSTabRestored, r=annyG

Comment 25

[u608768]

Test is already skipped, so I've landed it with the annotation pointing to bug 1717872 instead. Can confirm that the session restore parts of the test are fixed and the manual STR from comment #0 pass with Fission enabled.

Comment 26

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/648737f096f7
https://hg.mozilla.org/mozilla-central/rev/28a8b1e0114f