1614847 - tabs.update with {loadReplace: true} on tabs with moz-extension URLs broken in Firefox 74

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[anon]

User Agent: Mozilla/5.0 (Windows NT 6.3; Win64; x64; rv:74.0) Gecko/20100101 Firefox/74.0

Steps to reproduce:

1. In Firefox 73, install the 'Tab Suspender' extension: https://addons.mozilla.org/en-US/firefox/addon/ff-tab-suspender/
2. In this extensions Options page, deselect 'Automatic Suspend' and click 'Save'. This extension doesn't use native tab discarding, but instead updates a tab to a moz-extension URL when suspended, using the loadReplace property. If the page of a suspended tab is clicked on, the tab will be updated back to the original URL using the loadReplace property.
3. Create a new tab, load an https URL in it, select 'Tab Suspender' in that tab's context menu, and then select 'Suspend'.
4. Click anywhere on the page to restore/reload it.
5. Suspend the tab again, go to a new URL in the same tab, press the back button, restore the suspended tab, and then press the forward button.
6. Repeat the previous steps using Firefox 74.

Actual results:

On step 4) with Firefox 73, the moz-extension URL is removed from the tab's history (the back button is unavailable). On step 5) in Firefox 73, the forward arrow is available. In Firefox 74, the back arrow isn't removed on step 4) and the forward arrow is lost on step 5).

Expected results:

In Firefox 74, a tabs.update with the loadReplace property set to true on a tab with a moz-extension URL behaves like a tabs.update without the property set. The loadReplace property should be taken into account, like it was before Firefox 74. Note that because of https://bugzilla.mozilla.org/show_bug.cgi?id=1436321 , loadReplace is necessary to remove moz-extension URLs from a tabs history.

Comment 1

[Daniel Orban]

Hello,
I have managed to reproduce the issue on the latest Nightly (75.0a1/20200217095003), Beta (74.0b4/20200216164042), under Windows 10 Pro 64-bit and macOS Catalina 10.15.
The back arrow is not removed after the page is restored and the forward arrow is lost while performing the step 5.

Comment 2

[Pascal Chevrel:pascalc]

Shane, could this bug be triaged and prioritized please? Thanks

Comment 3

[Miruna Curtean]

Attachment: Regression behavior.mp4

One note in regards to the repro of this issue, particularly the actual results after step 4) is performed - before and after the regression the back button is available, however after the regression occurs when right clicking on the back button to show history the discarded tab is also listed.
This does not occur before the regression and if the user clicks on the back button they are returned to the new tab.
Attached video with actual results after regression.

As for the regression window, these are the results:

Last good build_date: 2020-01-10 01:45:32.146000
changeset: 325da10270e4bbc644df0fd29a6c3f6ef536d262
pushlog_url: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=325da10270e4bbc644df0fd29a6c3f6ef536d262&tochange=558a2ae20a30f33cc1b042dab27ef04eccb4ea58
repo_name: autoland
repo_url: https://hg.mozilla.org/integration/autoland
task_id: bXHdGMkhTYSIrtqzsfuWvA

First bad build_date: 2020-01-10 01:39:31.316000.
changeset: e166c43d60e363fe86615b0580d34f179db510b3
pushlog_url: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=325da10270e4bbc644df0fd29a6c3f6ef536d262&tochange=e166c43d60e363fe86615b0580d34f179db510b3
repo_name: autoland
repo_url: https://hg.mozilla.org/integration/autoland
task_id: Vx2OsGpiQqarrmHMyzSy0w

If no more details are needed please remove the regression tags.

Comment 4

[Tomislav Jovanovic :zombie]

Hey Matt, this seem to be from one of your patches for bug 1601779 or bug 1603196, could you please take a look at what's going on?

Our code in question just passes LOAD_FLAGS_REPLACE_HISTORY to loadURI:
https://searchfox.org/mozilla-central/rev/df94cd5b/browser/components/extensions/parent/ext-tabs.js#833

(separately, we apparently don't have a test for what loadReplace is supposed to do)

Comment 5

[Shane Caraveo (:mixedpuppy)]

I just conflicted with Tomislav, but have some additional info that shows it is not specific to the addon mentioned in OP:

As a note, this can be replicated with other tab discarding addons. I tested with two[1][2]. The quick STR:

1. Add a tab discarding addon.
2. open a new tab, in this tab:
3. go to site A via urlbar
4. go to site B via urlbar
5. click back button
6. change to any other tab
7. "discard" the tab from #2 using the context menu on that tab.
8. go back to the tab from #2 so it reloads

At this point, the "forward" history has been lost (in tab from #2)

[1] https://addons.mozilla.org/en-US/firefox/addon/discard-tab
[2] https://addons.mozilla.org/en-US/firefox/addon/unload-tab-from-context-menu

Comment 6

[Shane Caraveo (:mixedpuppy)]

Another thing to note, "discard-tab" uses our API browser.tabs.discard which uses functionality in tabbrowser itself. I just created another STR that uses no addon:

1. start firefox
2. create new tab
3. visit site A
4. visit site B
5. click back
6. select some other tab
7. restart firefox
8. click on tab created in #2

At this point, forward history is lost.

Given I can reproduce with our own discard functionality, I'm moving this to dom:navigation assuming it is a regression from one of the two mentioned bugs.

Comment 7

[Matt Woodrow (:mattwoodrow)]

I can reproduce the bug as originally filed, but I'm not able to reproduce either of the alternate STR from comments 5 and 6.

Comment 8

[Matt Woodrow (:mattwoodrow)]

Attachment: Bug 1614847 - Expand test for load types to include a cross-origin case that fails with fission. r?nika

Comment 9

[Matt Woodrow (:mattwoodrow)]

Attachment: Bug 1614847 - Construct nsDocShellLoadState in ContentChild to avoid needing to pass excess parameters into nsDocShell. r?nika

Depends on D63266

Comment 10

[Matt Woodrow (:mattwoodrow)]

Attachment: Bug 1614847 - Copy the load type across to the new nsDocShellLoadState. r?nika

Depends on D63267

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c182ab2cc8ad
Expand test for load types to include a cross-origin case that fails with fission. r=nika
https://hg.mozilla.org/integration/autoland/rev/ab59aa76251d
Construct nsDocShellLoadState in ContentChild to avoid needing to pass excess parameters into nsDocShell. r=nika
https://hg.mozilla.org/integration/autoland/rev/332b68c9da84
Copy the load type across to the new nsDocShellLoadState. r=nika

Comment 13

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/c182ab2cc8ad
https://hg.mozilla.org/mozilla-central/rev/ab59aa76251d
https://hg.mozilla.org/mozilla-central/rev/332b68c9da84

Comment 14

[Julien Cristau [:jcristau]]

Should we uplift to beta to avoid shipping this regression?

Comment 15

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 9127414 [details]
Bug 1614847 - Expand test for load types to include a cross-origin case that fails with fission. r?nika

Beta/Release Uplift Approval Request

• User impact if declined: Incorrect session history for extension pages in some cases
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky):
• String changes made/needed:

Comment 16

[Pascal Chevrel:pascalc]

Comment on attachment 9127414 [details]
Bug 1614847 - Expand test for load types to include a cross-origin case that fails with fission. r?nika

Fix for a 74 regression, uplift approved for 74 beta 9, thanks.

Comment 17

[Dorel Luca [:dluca]]

https://hg.mozilla.org/releases/mozilla-beta/rev/535c371bcf7c
https://hg.mozilla.org/releases/mozilla-beta/rev/f3993b8584bd
https://hg.mozilla.org/releases/mozilla-beta/rev/91abdb25eeeb

Comment 18

[juraj.masiar]

Was this already merged to beta 74.0b9 / nightly?
Because I can still reproduce the issue from duplicate bug 1615302