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)
Firefox
Session Restore
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)
21.12 KB,
patch
|
smacleod
:
review+
|
Details | Diff | Splinter Review |
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)
Comment 1•10 years ago
|
||
(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)
Reporter | ||
Comment 2•10 years ago
|
||
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.
Assignee | ||
Updated•10 years ago
|
OS: Linux → All
Hardware: x86_64 → All
Assignee | ||
Comment 3•10 years ago
|
||
Assignee | ||
Comment 4•10 years ago
|
||
Putting into backlog to unblock e10s work/dogfooding.
Flags: firefox-backlog+
Whiteboard: p=1 s=it-32c-31a-30b.1 [qa?]
Assignee | ||
Comment 5•10 years ago
|
||
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)
Assignee | ||
Updated•10 years ago
|
Whiteboard: p=1 s=it-32c-31a-30b.1 [qa?] → p=2 s=it-32c-31a-30b.1 [qa?]
Assignee | ||
Comment 6•10 years ago
|
||
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)
Assignee | ||
Comment 7•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=1ec8aacfb01f
Comment 8•10 years ago
|
||
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?
Assignee | ||
Comment 9•10 years ago
|
||
(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...
Assignee | ||
Comment 10•10 years ago
|
||
Attachment #8416863 -
Attachment is obsolete: true
Attachment #8416863 -
Flags: review?(smacleod)
Attachment #8417214 -
Flags: review?(smacleod)
Assignee | ||
Comment 11•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=3a9719ea8694
Reporter | ||
Comment 12•10 years ago
|
||
Thanks for working on this, Tim!
Comment 13•10 years ago
|
||
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+
Assignee | ||
Comment 14•10 years ago
|
||
(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
Updated•10 years ago
|
Flags: needinfo?(jbecerra)
Updated•10 years ago
|
Flags: needinfo?(jbecerra)
Whiteboard: p=2 s=it-32c-31a-30b.1 [qa?] → p=2 s=it-32c-31a-30b.1 [qa-]
Assignee | ||
Comment 16•10 years ago
|
||
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).
Assignee | ||
Comment 17•10 years ago
|
||
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+]
Updated•10 years ago
|
QA Contact: cornel.ionce
Comment 18•10 years ago
|
||
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!]
Assignee | ||
Comment 19•10 years ago
|
||
Thank you!
You need to log in
before you can comment on or make changes to this bug.
Description
•