Closed Bug 1002843 Opened 10 years ago Closed 10 years ago

Session restore code runs before browser.js onload handler

Categories

(Firefox :: Session Restore, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 32

People

(Reporter: billm, Assigned: ttaubert)

References

Details

(Whiteboard: p=2 s=it-32c-31a-30b.1 [qa!])

Attachments

(1 file, 2 obsolete files)

I found this a little disturbing. When restoring a session containing multiple windows, we need to open new windows. We store the state to be restored into the new window in _statesToRestore. When the new window opens, the onOpen handler fires. That calls onLoad, which eventually calls restoreTabs.

The problem is that gBrowserInit.onLoad hasn't fired at this time. Consequently, gMultiProcessBrowser hasn't been set and so we always restore the tabs as if they were non-remote. It also seems strange to me that we're calling a function called onLoad before the window "load" event has fired.

Do you have any ideas about this Tim? Could we maybe install a "load" listener in onOpen and call onLoad only when it fires?
Flags: needinfo?(ttaubert)
(In reply to Bill McCloskey (:billm) from comment #0)
> Do you have any ideas about this Tim? Could we maybe install a "load"
> listener in onOpen and call onLoad only when it fires?

That's exactly what we're doing?
http://mxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/src/SessionStore.jsm?rev=ec9e918e47dd#986
Flags: needinfo?(ttaubert)
Oh, I see. I guess the problem is that, for whatever reason, the session restore load listener is running before the one in browser.js. I've seen this problem before and it's really annoying. I wish there were some way to guarantee that one load listener runs before another.

Anyway, we need to find a way to fix this.
OS: Linux → All
Hardware: x86_64 → All
Assignee: nobody → ttaubert
Status: NEW → ASSIGNED
Attachment #8416859 - Flags: review?(smacleod)
Putting into backlog to unblock e10s work/dogfooding.
Flags: firefox-backlog+
Whiteboard: p=1 s=it-32c-31a-30b.1 [qa?]
Comment on attachment 8416859 [details] [diff] [review]
0001-Bug-1002843-Wait-until-important-parts-have-been-ini.patch

Need to fix some things.
Attachment #8416859 - Flags: review?(smacleod)
Whiteboard: p=1 s=it-32c-31a-30b.1 [qa?] → p=2 s=it-32c-31a-30b.1 [qa?]
This patch addresses the fact that there are other window types than browser windows. Some very old tests had to be fixed that assumed SessionStore is accessible for a window from when that window's load event fires.
Attachment #8416859 - Attachment is obsolete: true
Attachment #8416863 - Flags: review?(smacleod)
What's the purpose of onOpen and its load event listener at this point? Couldn't SessionStore.jsm just globally observe browser-window-before-show?
(In reply to Dão Gottwald [:dao] from comment #8)
> What's the purpose of onOpen and its load event listener at this point?
> Couldn't SessionStore.jsm just globally observe browser-window-before-show?

Good point, that didn't occur to me. Thanks for that suggestion! I'll need to touch the patch anyway again looking at the try run...
Attachment #8416863 - Attachment is obsolete: true
Attachment #8416863 - Flags: review?(smacleod)
Attachment #8417214 - Flags: review?(smacleod)
Thanks for working on this, Tim!
Comment on attachment 8417214 [details] [diff] [review]
0001-Bug-1002843-Wait-until-important-parts-have-been-ini.patch, v3

Review of attachment 8417214 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/browser.js
@@ +859,5 @@
>      }
>  
>      // Certain kinds of automigration rely on this notification to complete their
>      // tasks BEFORE the browser window is shown.
> +    // SessionStore uses it to restore tabs into windows *after* critical parts

This comment wrapping is really strange to me, were you just trying to avoid altering blame of the comment? I think I'd prefer it just be joined on the previous line
Attachment #8417214 - Flags: review?(smacleod) → review+
(In reply to Steven MacLeod [:smacleod] from comment #13)
> This comment wrapping is really strange to me, were you just trying to avoid
> altering blame of the comment? I think I'd prefer it just be joined on the
> previous line

Yeah, agreed and fixed. I kind of felt like doing it differently that moment.

https://hg.mozilla.org/integration/fx-team/rev/fb2f46c5f6b1
https://hg.mozilla.org/mozilla-central/rev/fb2f46c5f6b1
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 32
Flags: needinfo?(jbecerra)
Flags: needinfo?(jbecerra)
Whiteboard: p=2 s=it-32c-31a-30b.1 [qa?] → p=2 s=it-32c-31a-30b.1 [qa-]
To verify this, you can (as described/hidden in comment #0) flip browser.tabs.remote.autostart and check that with a multi-window session every tab in every window is restored as a remote tab after restart. This wasn't the case before this patch landed, only the initial window had remote tabs, tabs from every other window were restored as non-remote (no underlined tab label).
Can we please verify this? Should be easy, see comment #16 :)
Whiteboard: p=2 s=it-32c-31a-30b.1 [qa-] → p=2 s=it-32c-31a-30b.1 [qa+]
QA Contact: cornel.ionce
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:32.0) Gecko/20100101 Firefox/32.0
Mozilla/5.0 (Windows NT 6.3; rv:32.0) Gecko/20100101 Firefox/32.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:32.0) Gecko/20100101 Firefox/32.0
Mozilla/5.0 (X11; Linux i686; rv:32.0) Gecko/20100101 Firefox/32.0

Verified fixed using latest Nightly, build ID: 20140508030203.
Flipped the "browser.tabs.remote.autostart" pref and restarting several times with multi-window sessions.
Each tab from every window was restored as a remote (underlined) tab after the browser restart.
Status: RESOLVED → VERIFIED
Whiteboard: p=2 s=it-32c-31a-30b.1 [qa+] → p=2 s=it-32c-31a-30b.1 [qa!]
Thank you!
Depends on: 1009599
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: