Don't rely on "domwindowclosed" being fired in the expected order

RESOLVED FIXED in Firefox 7

Status

Firefox Graveyard
Panorama
RESOLVED FIXED
6 years ago
a year ago

People

(Reporter: ttaubert, Assigned: ttaubert)

Tracking

Trunk
Firefox 7
Dependency tree / graph
Bug Flags:
in-testsuite +

Details

Attachments

(1 attachment, 3 obsolete attachments)

Panorama saves all data when receiving "domwindowclosed". The same event is used by session store to remove a window from SS tracking. If SS is notified before Panorama an exception is thrown.

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1304644154.1304648994.11579.gz
Blocks: 653099
Assignee: nobody → tim.taubert
Status: NEW → ASSIGNED
Created attachment 530988 [details] [diff] [review]
patch v1

Passed try:

http://tbpl.mozilla.org/?tree=Try&pusher=tim.taubert@gmx.de&rev=617e7032720c
Attachment #530988 - Flags: feedback?(raymond)
Comment on attachment 530988 [details] [diff] [review]
patch v1

looks good!
Attachment #530988 - Flags: feedback?(raymond) → feedback+
Attachment #530988 - Flags: review?(paul)
Attachment #530988 - Flags: review?(dao)
Created attachment 534726 [details] [diff] [review]
patch v2

Unrotted.
Attachment #530988 - Attachment is obsolete: true
Attachment #530988 - Flags: review?(paul)
Attachment #530988 - Flags: review?(dao)
Attachment #534726 - Flags: review?(paul)
Attachment #534726 - Flags: review?(dao)
Comment on attachment 534726 [details] [diff] [review]
patch v2

>--- a/browser/components/sessionstore/src/nsSessionStore.js
>+++ b/browser/components/sessionstore/src/nsSessionStore.js
>@@ -868,17 +868,20 @@ SessionStoreService.prototype = {
>     // ignore windows not tracked by SessionStore
>     if (!aWindow.__SSi || !this._windows[aWindow.__SSi]) {
>       return;
>     }
>     
>     if (this.windowToFocus && this.windowToFocus == aWindow) {
>       delete this.windowToFocus;
>     }
>-    
>+
>+    // Uninitialize Panorama so that it can save all its data to session store.
>+    aWindow.TabView.uninit();

This seems like a failure to provide the right API -- if we don't have a way for someone to externally store data at the right time, we should probably create one.
Attachment #534726 - Flags: review?(dao) → review-
Attachment #534726 - Flags: review?(paul)
Created attachment 534788 [details] [diff] [review]
patch v3
Attachment #534726 - Attachment is obsolete: true
Attachment #534788 - Flags: review?(dao)
Comment on attachment 534788 [details] [diff] [review]
patch v3

So you're calling aWindow.TabView.saveData instead of aWindow.TabView.uninit -- this isn't conceptually different from the previous patch. The sessionstore service should provide hooks for tabview and other API consumers to do their work.
Attachment #534788 - Flags: review?(dao) → review-
I agree with Dão here. Calling into Panorama here felt wrong when I looked at this before. Session Restore isn't supposed to know anything about Panorama's existence (with one small exception but that's marked with a nice "XXX we shouldn't do this" comment and is unavoidable right now).

So we should probably do like what we do for tabs (SSTabClosing), which is fired at the beginning of sessionstore processing the TabClose event. We can add SSWindowClosing. We're starting to pollute the event space there (we added SSWindowState{Busy|Ready} for FF4), but that's ok.

Tim - want to file a new bug blocking this for the new event?
(In reply to comment #6)
> Comment on attachment 534788 [details] [diff] [review] [review]
> patch v3
> 
> So you're calling aWindow.TabView.saveData instead of aWindow.TabView.uninit
> -- this isn't conceptually different from the previous patch. The
> sessionstore service should provide hooks for tabview and other API
> consumers to do their work.

Yeah, sorry I completely got you wrong (obviously).
(In reply to comment #7)
> So we should probably do like what we do for tabs (SSTabClosing), which is
> fired at the beginning of sessionstore processing the TabClose event. We can
> add SSWindowClosing. We're starting to pollute the event space there (we
> added SSWindowState{Busy|Ready} for FF4), but that's ok.

Sounds good.

> Tim - want to file a new bug blocking this for the new event?

Yep, bug 659591 :)
Depends on: 659591
Created attachment 535043 [details] [diff] [review]
patch v4
Attachment #534788 - Attachment is obsolete: true
Attachment #535043 - Flags: review?(dao)

Updated

6 years ago
Attachment #535043 - Flags: review?(dao) → review+
Comment on attachment 535043 [details] [diff] [review]
patch v4

Passed try:

http://tbpl.mozilla.org/?tree=Try&pusher=tim.taubert@gmx.de&rev=42be061619c1
bugspam
No longer blocks: 653099
bugspam
Blocks: 660175
http://hg.mozilla.org/integration/mozilla-inbound/rev/61e1aa1afa09
Whiteboard: [inbound]
Backed out due to mochitest-other orange.
Whiteboard: [inbound]
http://hg.mozilla.org/integration/mozilla-inbound/rev/7232c7714d81
Whiteboard: [inbound]
http://hg.mozilla.org/mozilla-central/rev/7232c7714d81
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → Firefox 7
The results are fine in:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1314786255.1314789873.815.gz

Are there other steps/guidelines I could use before setting this issue to verified?
(In reply to Virgil Dicu [QA] from comment #18)
> Are there other steps/guidelines I could use before setting this issue to
> verified?

This is a rather technical issue which can't be easily verified manually. It's definitely fixed in conjunction with bug 659591 :)
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.