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

Status: RESOLVED FIXED

(Firefox Graveyard :: Panorama, defect)

Description

[Tim Taubert [:ttaubert] (inactive)]

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

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v1

Passed try:

http://tbpl.mozilla.org/?tree=Try&pusher=[e-mail]&rev=617e7032720c

Comment 2

[Raymond Lee [:raymondlee]]

Comment on attachment 530988 [details] [diff] [review]
patch v1

looks good!

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v2

Unrotted.

Comment 4

[Dão Gottwald [: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.

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v3

Comment 6

[Dão Gottwald [: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.

Comment 7

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

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?

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

(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).

Comment 9

[Tim Taubert [:ttaubert] (inactive)]

(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 :)

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v4

Comment 11

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 535043 [details] [diff] [review]
patch v4

Passed try:

http://tbpl.mozilla.org/?tree=Try&pusher=[e-mail]&rev=42be061619c1

Comment 12

[Tim Taubert [:ttaubert] (inactive)]

bugspam

Comment 13

[Tim Taubert [:ttaubert] (inactive)]

bugspam

Comment 14

[Tim Taubert [:ttaubert] (inactive)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/61e1aa1afa09

Comment 15

[Boris Zbarsky [:bzbarsky]]

Backed out due to mochitest-other orange.

Comment 16

[Tim Taubert [:ttaubert] (inactive)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/7232c7714d81

Comment 17

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/7232c7714d81

Comment 18

[Virgil Dicu [:virgil] [QA]]

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?

Comment 19

[Tim Taubert [:ttaubert] (inactive)]

(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 :)