Closed Bug 1596738 Opened 5 years ago Closed 4 years ago

Session restore focuses the browser in fission, preventing re-enabling browser_newwindow_focus.js

Categories

(Firefox :: Session Restore, defect, P2)

Product:
Firefox
Component:
Session Restore
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 80
Project Flags:
Fission Milestone M4.1
Tracking Flags:
Tracking Status
firefox-esr68 --- unaffected
firefox-esr78 --- disabled
firefox78 --- disabled
firefox79 --- disabled
firefox80 --- fixed

People

(Reporter: enndeakin, Assigned: u608768)

References

(Blocks 2 open bugs, Regressed 1 open bug)

Details

(Keywords: regression)

Attachments

(1 file)

Bug 1596738 - Don't focus the tab's content area for restores triggered by DocumentChannel process switches, r?dao
47 bytes, text/x-phabricator-request
Details | Review

The test browser_newwindow_focus.js fails with fission enabled with:

URLBar should be focused - [object XULFrameElement] == {}

The <browser> is focused instead of the urlbar.

The code at the following is causing the focus change:

https://searchfox.org/mozilla-central/rev/cac340f10816707e91c46db6d285f80259420f07/browser/components/sessionstore/SessionStore.jsm#4792

The underlying difference from non-fission is that at:
https://searchfox.org/mozilla-central/rev/cac340f10816707e91c46db6d285f80259420f07/browser/components/sessionstore/SessionStore.jsm#2739

the remoteType is 'webIsolated=http://example.com' but the currentRemoteType is 'web'.

With fission disabled, both have a value of 'web'.

Tentatively moving all bugs whose summaries mention "Fission" (or other Fission-related keywords) but are not assigned to a Fission Milestone to the "?" triage milestone.

This will generate a lot of bugmail, so you can filter your bugmail for the following UUID and delete them en masse:

0ee3c76a-bc79-4eb2-8d12-05dc0b68e732

Fission Milestone: --- → ?
Fission Milestone: ? → M4.1

Mike, this bug is a blocker for Fission's current milestone (M4.1 aka "fix all the mochitests"), but it's currently unassigned. The Fission team is hoping teams will fix their mochitests for Fission before the end of Q1 (75 or 76 Nightly).

Will your team be able to prioritize this bug for Q1? If you don't think this mochitest failure should block shipping Fission, just let me know.

If you have questions for Fission engineers, you can reach them in the #fission channel on Slack or Riot.

Flags: needinfo?(mdeboer)
Type: task → defect
Priority: -- → P2
Severity: normal → S4
Flags: needinfo?(mdeboer)

(In reply to Neil Deakin from comment #0)

the remoteType is 'webIsolated=http://example.com' but the currentRemoteType is 'web'.

With fission disabled, both have a value of 'web'.

Neil, is this webIsolated focus issue still happening? Is the bug that remoteType is webIsolated and should be web? Or is the bug how the SessionStore code or test handles the new webIsolated remoteType?

Flags: needinfo?(enndeakin)

Neil, is this webIsolated focus issue still happening? Is the bug that remoteType is webIsolated and should be web? Or is the bug how the SessionStore code or test handles the new webIsolated remoteType?

The test still fails, but the code has changed here quite a bit. But disabling the focusing step at https://searchfox.org/mozilla-central/rev/61fceb7c0729773f544a9656f474e36cd636e5ea/browser/components/sessionstore/SessionStore.jsm#4896 makes the test pass. I don't really know why the session store is involved here. Best to ask someone with more familiarity with session store but I don't know who that would be.

Flags: needinfo?(enndeakin)

(In reply to Neil Deakin from comment #4)

I don't really know why the session store is involved here.

Likely via performProcessSwitch: https://searchfox.org/mozilla-central/rev/61fceb7c0729773f544a9656f474e36cd636e5ea/browser/base/content/tabbrowser.js#5724

(In reply to Dão Gottwald [::dao] from comment #5)

Likely via performProcessSwitch: https://searchfox.org/mozilla-central/rev/61fceb7c0729773f544a9656f474e36cd636e5ea/browser/base/content/tabbrowser.js#5724

Paul, can you please take a look at this bug when you get a chance? Nika recommended you because this test failure is probably related to process switching, which you have experience with.

This bug is a Fission M4.1 mochitest failure, so it's higher priority than Fission M6a bugs (but not a higher priority than your allowLinkedWebInFileUriProcess uplifts).

Flags: needinfo?(pbone)

Tentatively assigning to Paul.

Assignee: nobody → pbone

Yep, I'll take a look. I'm going to leave the NI here so I remember.

Assignee: pbone → kmadan
Flags: needinfo?(pbone)

This is happening because we're only process switching the load when fission is enabled. SessionStoreInternal.restoreTabContent (this is where we focus the new browser) is called near the end of this process. So any focusing that happens before the process switch is done is essentially thrown away, which is what we're seeing in the test.

Full js stack:

restoreTabContent@resource:///modules/sessionstore/SessionStore.jsm:4948:7
restoreTab@resource:///modules/sessionstore/SessionStore.jsm:4824:14
finishTabRemotenessChange@resource:///modules/sessionstore/SessionStore.jsm:6088:10
finishTabRemotenessChange@resource:///modules/sessionstore/SessionStore.jsm:540:26
finishBrowserRemotenessChange@chrome://browser/content/tabbrowser.js:5761:20
finishChangeRemoteness@chrome://global/content/elements/browser-custom-element.js:1908:20

nsIBrowser::FinishChangeRemoteness is called here.

Removing the code that focuses the new browser seems to fix the broken test case. It also doesn't break the working test case (which checks that we correctly focus content-opened browsers), so I'm unsure if this code actually does anything.

You'll also want to check that you don't regress bug 1410591.

From a try push with that block removed entirely:

[task 2020-07-07T18:15:32.400Z] 18:15:32     INFO - TEST-START | browser/base/content/test/general/browser_newwindow_focus.js
[task 2020-07-07T18:15:34.965Z] 18:15:34     INFO - GECKO(8569) | MEMORY STAT | vsize 3657MB | residentFast 648MB | heapAllocated 328MB
[task 2020-07-07T18:15:34.965Z] 18:15:34     INFO - TEST-OK | browser/base/content/test/general/browser_newwindow_focus.js | took 2567ms
...
[task 2020-07-07T18:38:44.770Z] 18:38:44     INFO - TEST-START | browser/components/sessionstore/test/browser_focus_after_restore.js
[task 2020-07-07T18:38:45.202Z] 18:38:45     INFO - GECKO(17726) | MEMORY STAT | vsize 3447MB | residentFast 552MB | heapAllocated 258MB
[task 2020-07-07T18:38:45.203Z] 18:38:45     INFO - TEST-OK | browser/components/sessionstore/test/browser_focus_after_restore.js | took 435ms

(both with and without fission)

Now to figure out why the test from bug 1410591 is working without that code...

Kshav, you may also need to test that Bug 1634272 is still okay. Bug 1643578 might also be releated.

Thanks for taking this.

See Also: → 1410591, 1634272, 1643578

(In reply to Paul Bone [:pbone] from comment #12)

Thanks for the extra tests!

you may also need to test that Bug 1634272 is still okay.

  • From that same comment #11 try push:

    • without fission:

      [task 2020-07-07T19:00:17.109Z] 19:00:17     INFO - TEST-START | browser/base/content/test/tabs/browser_navigate_home_focuses_addressbar.js
[task 2020-07-07T19:00:17.678Z] 19:00:17     INFO - GECKO(14792) | JavaScript error: , line 0: uncaught exception: Object
[task 2020-07-07T19:00:17.678Z] 19:00:17     INFO - GECKO(14792) | JavaScript error: , line 0: uncaught exception: Object
[task 2020-07-07T19:00:17.678Z] 19:00:17     INFO - GECKO(14792) | JavaScript error: , line 0: uncaught exception: Object
[task 2020-07-07T19:00:17.780Z] 19:00:17     INFO - GECKO(14792) | MEMORY STAT | vsize 3777MB | residentFast 427MB | heapAllocated 160MB
[task 2020-07-07T19:00:17.782Z] 19:00:17     INFO - TEST-OK | browser/base/content/test/tabs/browser_navigate_home_focuses_addressbar.js | took 672ms

    • and with fission:

      [task 2020-07-07T18:32:09.589Z] 18:32:09     INFO - TEST-START | browser/base/content/test/tabs/browser_navigate_home_focuses_addressbar.js
[task 2020-07-07T18:32:10.295Z] 18:32:10     INFO - GECKO(7290) | JavaScript error: resource:///actors/ContentSearchParent.jsm, line 629: TypeError: can't access property "addEventListener", browser is null
[task 2020-07-07T18:32:10.352Z] 18:32:10     INFO - GECKO(7290) | MEMORY STAT | vsize 3770MB | residentFast 396MB | heapAllocated 122MB
[task 2020-07-07T18:32:10.352Z] 18:32:10     INFO - TEST-OK | browser/base/content/test/tabs/browser_navigate_home_focuses_addressbar.js | took 758ms

    I see the errors locally both with and without the code change, so I think they're unrelated.

Bug 1643578 might also be related.

  • I'm seeing the expected results (urlbar is focused) with the STR from bug 1643578 comment #0 with these changes.

(In reply to :kashav from comment #11)

Now to figure out why the test from bug 1410591 is working without that code...

I think it's the call to nsIFocusManager::SetFocus in tabbrowser._adjustFocusAfterTabSwitch that's focusing the restored tab in that test.

JS stack (line numbers might be off by a bit because of log statements):

_adjustFocusAfterTabSwitch@chrome://browser/content/tabbrowser.js:1342
updateDisplay@resource:///modules/AsyncTabSwitcher.jsm:479
postActions@resource:///modules/AsyncTabSwitcher.jsm:673
handleEvent@resource:///modules/AsyncTabSwitcher.jsm:1117
queueUnload@resource:///modules/AsyncTabSwitcher.jsm:1052
requestTab@resource:///modules/AsyncTabSwitcher.jsm:1048
updateCurrentBrowser@chrome://browser/content/tabbrowser.js:988
_setupEventListeners@chrome://browser/content/tabbrowser.js:5265
set selectedIndex@chrome://global/content/elements/tabbox.js:197
set selectedPanel@chrome://global/content/elements/tabbox.js:216
set selectedIndex@chrome://global/content/elements/tabbox.js:547
set selectedItem@chrome://global/content/elements/tabbox.js:567
set selectedTab@chrome://global/content/elements/tabbox.js:87
selectedTab@chrome://browser/content/tabbrowser.js:293
addMultipleTabs@chrome://browser/content/tabbrowser.js:2918
ssi_restoreWindow@resource:///modules/sessionstore/SessionStore.jsm:4377
_restoreWindowsFeaturesAndTabs@resource:///modules/sessionstore/SessionStore.jsm:4526
_restoreWindowsInReversedZOrder@resource:///modules/sessionstore/SessionStore.jsm:4550
ssi_restoreWindows@resource:///modules/sessionstore/SessionStore.jsm:4611
ssi_restoreWindows@resource:///modules/sessionstore/SessionStore.jsm:4605

(sent comment #14 twice)

We run this code near the end of a remoteness change. It causes the browser to
steal focus from any element that was manually focused prior to the completion
of the load. This is particularly a problem for fission loads because every
cross-origin navigation requires a process switch.

This was originally added in bug 1410591 to ensure that we focus the content
area instead of the address bar after a session restore. It appears that this
is now handled elsewhere, which makes this code redundant.

Attachment #9162271 - Attachment description: Bug 1596738 - Don't explicitly focus the restored tab's content area, r?dao → Bug 1596738 - Don't focus the tab's content area for remoteness changes, r?dao
Attachment #9162271 - Attachment description: Bug 1596738 - Don't focus the tab's content area for remoteness changes, r?dao → Bug 1596738 - Don't focus the tab's content area for navigation-driven restores, r?dao
Pushed by kmadan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d2e762d32fb6
Don't focus the tab's content area for navigation-driven restores, r=dao

I think this is also the cause of bug 1651372, so it may be good to test and see if a fix to this issue fixes that one as well.

Blocks: 1643578

These new duplicate bugs (like bug 1651372) are quite serious, luckily they are still in Nightly.
If the fix here won't land before Beta is released (less than 2 weeks from now), could someone revert the changes? There is a regression window in that bug.

ya I don't think P2/S4 really reflects the gravity of the custom new tab page issue, it effectively makes a huge number of extensions unusable

Keywords: regression
Attachment #9162271 - Attachment description: Bug 1596738 - Don't focus the tab's content area for navigation-driven restores, r?dao → Bug 1596738 - Don't focus the tab's content area for restores triggered by DocumentChannel process switches, r?dao
Pushed by kmadan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1628b650e8fb
Don't focus the tab's content area for restores triggered by DocumentChannel process switches, r=dao,nika,mixedpuppy

https://hg.mozilla.org/mozilla-central/rev/1628b650e8fb

Status: NEW → RESOLVED
Closed: 4 years ago
status-firefox80: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 80

Did anyone check if that revision works the same way for custom home page on new window? The same thing was observed creating a new window with a custom home page, where the URL is not just about:newtab, but would be set in prefs to something like moz-extension://blahblahblah.html. Focus being directed to the browser content overriding the urlbar focus

(In reply to shmediaproductions from comment #28)

Did anyone check if that revision works the same way for custom home page on new window? The same thing was observed creating a new window with a custom home page, where the URL is not just about:newtab, but would be set in prefs to something like moz-extension://blahblahblah.html. Focus being directed to the browser content overriding the urlbar focus

I tested the patch with the STR from bug 1643578, bug 1652293, and bug 1653069. Bug 1643578 covers the case you're talking about (I ran into some issues writing an actual test for it, but that'll happen soon). Just to be clear, non-extension home/newtab urls still behave the same they always have (i.e., no urlbar focus).

The patch will be in tomorrow's nightly.

Flags: needinfo?(kmadan)

Thanks, I guess I never noticed that, I don't make new windows very often. I was always just manually entering my extension's newtab.html URL into the home page preference. So if I change the homepage to a local extension document, via chrome_settings_overrides in manifest.json, does that mean it WILL focus the urlbar when I create a new window? I just want the new window behavior to be the same as the new tab behavior with my extension.

Blocks: 1626507

(In reply to shmediaproductions from comment #30)

I was always just manually entering my extension's newtab.html URL into the home page preference.

This is fine and should result in the urlbar being focused for new windows.

So if I change the homepage to a local extension document, via chrome_settings_overrides in manifest.json, does that mean it WILL focus the urlbar when I create a new window?

This is also fine and should also result in the urlbar being focused for new windows.

As long as the homepage url is a moz-extension:// document (regardless of how it's been set), the urlbar will get focus in new windows. I was only referring to non-extension urls (like https://example.com, etc).

Yeah just tested it and they both work correctly now, thanks

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

Attachment

General

Creator:
Created:
Updated:
Size: