Open Bug 1696815 Opened 2 months ago Updated 9 days ago

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

Categories

(Firefox :: Session Restore, defect)

defect

Tracking

()

ASSIGNED
Fission Milestone M7a

People

(Reporter: robwu, Assigned: annyG)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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).

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).

Fission Milestone: --- → M7
Flags: needinfo?(rob)

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)

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.

Flags: needinfo?(rob)
Fission Milestone: M7 → M7a

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

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?

Flags: needinfo?(tomica)

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.

Flags: needinfo?(tomica)

Kashav will test and confirm if this is still broken.

Flags: needinfo?(kmadan)

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

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

Flags: needinfo?(kmadan) → needinfo?(agakhokidze)

Might be fixed with the patches from bug 1702055.

Edit: it is not.

Assignee: nobody → agakhokidze
Status: NEW → ASSIGNED
Flags: needinfo?(agakhokidze)

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.

You need to log in before you can comment on or make changes to this bug.