Last Comment Bug 655269 - Don't rely on "domwindowclosed" being fired in the expected order
: Don't rely on "domwindowclosed" being fired in the expected order
Status: RESOLVED FIXED
:
Product: Firefox Graveyard
Classification: Graveyard
Component: Panorama (show other bugs)
: Trunk
: All All
: -- normal
: Firefox 7
Assigned To: Tim Taubert [:ttaubert]
:
Mentors:
Depends on: 659591
Blocks: 595020 655197 660175
  Show dependency treegraph
 
Reported: 2011-05-06 08:54 PDT by Tim Taubert [:ttaubert]
Modified: 2016-04-12 14:00 PDT (History)
8 users (show)
ehsan: in‑testsuite+
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch v1 (5.52 KB, patch)
2011-05-08 23:37 PDT, Tim Taubert [:ttaubert]
raymond: feedback+
Details | Diff | Splinter Review
patch v2 (5.41 KB, patch)
2011-05-24 04:34 PDT, Tim Taubert [:ttaubert]
dao+bmo: review-
Details | Diff | Splinter Review
patch v3 (9.55 KB, patch)
2011-05-24 08:58 PDT, Tim Taubert [:ttaubert]
dao+bmo: review-
Details | Diff | Splinter Review
patch v4 (6.93 KB, patch)
2011-05-25 05:44 PDT, Tim Taubert [:ttaubert]
dao+bmo: review+
Details | Diff | Splinter Review

Description Tim Taubert [:ttaubert] 2011-05-06 08:54:41 PDT
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 2 Raymond Lee [:raymondlee] 2011-05-09 01:35:55 PDT
Comment on attachment 530988 [details] [diff] [review]
patch v1

looks good!
Comment 3 Tim Taubert [:ttaubert] 2011-05-24 04:34:34 PDT
Created attachment 534726 [details] [diff] [review]
patch v2

Unrotted.
Comment 4 Dão Gottwald [:dao] 2011-05-24 06:34:12 PDT
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] 2011-05-24 08:58:37 PDT
Created attachment 534788 [details] [diff] [review]
patch v3
Comment 6 Dão Gottwald [:dao] 2011-05-24 09:47:29 PDT
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 reading much bugmail, email directly) 2011-05-24 10:44:43 PDT
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] 2011-05-25 02:48:57 PDT
(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] 2011-05-25 02:58:24 PDT
(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] 2011-05-25 05:44:52 PDT
Created attachment 535043 [details] [diff] [review]
patch v4
Comment 11 Tim Taubert [:ttaubert] 2011-05-26 03:27:18 PDT
Comment on attachment 535043 [details] [diff] [review]
patch v4

Passed try:

http://tbpl.mozilla.org/?tree=Try&pusher=tim.taubert@gmx.de&rev=42be061619c1
Comment 12 Tim Taubert [:ttaubert] 2011-05-27 02:22:19 PDT
bugspam
Comment 13 Tim Taubert [:ttaubert] 2011-05-27 02:27:13 PDT
bugspam
Comment 15 Boris Zbarsky [:bz] 2011-06-09 11:57:32 PDT
Backed out due to mochitest-other orange.
Comment 18 Virgil Dicu [:virgil] [QA] 2011-08-31 04:56:30 PDT
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] 2011-09-08 05:34:07 PDT
(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 :)

Note You need to log in before you can comment on or make changes to this bug.