The default bug view has changed. See this FAQ.

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)

(Assignee)

Description

6 years ago
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
(Assignee)

Updated

6 years ago
Blocks: 653099
(Assignee)

Updated

6 years ago
Assignee: nobody → tim.taubert
Status: NEW → ASSIGNED
(Assignee)

Comment 1

6 years ago
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+
(Assignee)

Updated

6 years ago
Attachment #530988 - Flags: review?(paul)
Attachment #530988 - Flags: review?(dao)
(Assignee)

Comment 3

6 years ago
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-
(Assignee)

Updated

6 years ago
Attachment #534726 - Flags: review?(paul)
(Assignee)

Comment 5

6 years ago
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?
(Assignee)

Comment 8

6 years ago
(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).
(Assignee)

Comment 9

6 years ago
(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
(Assignee)

Comment 10

6 years ago
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+
(Assignee)

Comment 11

6 years ago
Comment on attachment 535043 [details] [diff] [review]
patch v4

Passed try:

http://tbpl.mozilla.org/?tree=Try&pusher=tim.taubert@gmx.de&rev=42be061619c1
(Assignee)

Comment 12

6 years ago
bugspam
No longer blocks: 653099
(Assignee)

Comment 13

6 years ago
bugspam
Blocks: 660175
(Assignee)

Comment 14

6 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/61e1aa1afa09
Whiteboard: [inbound]
Backed out due to mochitest-other orange.
Whiteboard: [inbound]
(Assignee)

Comment 16

6 years ago
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?
(Assignee)

Comment 19

6 years ago
(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.