"winData is undefined(TypeError)" error on getWindowState called during window unload event

RESOLVED WONTFIX

Status

()

Firefox
Extension Compatibility
--
major
RESOLVED WONTFIX
2 years ago
a year ago

People

(Reporter: morac, Unassigned)

Tracking

({regression})

45 Branch
x86_64
Windows 7
regression
Points:
---

Firefox Tracking Flags

(firefox45 affected, firefox46 wontfix, firefox47 wontfix, firefox48 wontfix, firefox49 wontfix, firefox50 wontfix, firefox51 wontfix)

Details

Attachments

(1 attachment)

(Reporter)

Description

2 years ago
As of the 45 branch and up to and including the Trunk branch, calling the getWindowState SessionStore API call from inside a listener to the Window onunload event results in a "winData is undefined(TypeError)" error.

It appears that the window data is removed from the _windows object prior to the unload event completing.  

This started in Firefox 45.  In Firefox 44.0.2, this returned the data of the closing window.

This can be tested using the Session Manager addon by creating an "auto-save" window and then closing the window.  There is probably a simpler test case that can be made.  I'll see about doing that later.
(Reporter)

Comment 1

a year ago
Looks like Bug 1227444 caused this.
Blocks: 1227444
Has Regression Range: --- → yes
Has STR: --- → yes
status-firefox45: --- → affected
status-firefox46: --- → affected
status-firefox47: --- → affected
status-firefox48: --- → affected
Flags: needinfo?(mconley)
Keywords: regression
Hrm, yep. We're removing the winData entry in domwindowclosed, and not waiting for the unload handlers to complete.

Thanks for letting me know about this regression. I'll see if I can find some time to patch this. If you could provide the minimal test case you mentioned in comment 0, that'd be excellent.
Flags: needinfo?(mconley) → needinfo?(morac99-firefox2)
(Reporter)

Comment 3

a year ago
Created attachment 8730304 [details]
Stack trace of error.

I'm not sure if this is a minimal test case, but here's how to reproduce this using the Session Manager add-on, which is the easiest way to reproduce it.

1. Install Session Manager from - https://addons.mozilla.org/en-US/firefox/addon/session-manager/
2. Session Manager should install a blue disk icon to the location bar (if not, choose customize and drag it there).
3. Open a new browser window.
4. In that window either middle click the disk icon or click the arrow next to it, choose the Session Manager and then Save Current Window.
5. In the window that pops up either leave the default name or give it a new name.
6. Check the "Make this an auto-save session" check box.
7. Click Okay.
8. At this point the disk icon should have a yellow background.  If it does just close the window with the red X (Windows).

You'll get a pop-up with a stack trace that looks like the one attached.  Ignore the "file access error" part as that value is hard coded.  The actual exception is the line below that.

The source code of Session Manager addon is available on AMO.  It's not the easiest read, but the stack trace should help.  Basically the trace starts when the window unload event fires.  At the moment Session Manager uses the compatibility shims for windows, but I don't think that should make a difference here.


Is there another event I can listen to that fires before winData is cleared, but after the data for the closed window is saved?
Flags: needinfo?(morac99-firefox2)
(In reply to Michael Kraft [:morac] from comment #3)
> Is there another event I can listen to that fires before winData is cleared,
> but after the data for the closed window is saved?

So what's going on here is that the we close the window synchronously in the domwindowclose onClose handler (and clear the window data out), and kick off an async flush of the window to make sure we've got up to date information on what the state of the window is. When the async window flush completes, we then decide whether or not to save the window in the _closedWindows collection.

That means that the order of events is:

1) Window starts to close
2) Window data is cleared
3) Async flush begins
4) Closed window data, if saveable, is saved.

Since winData is cleared before the closed window is data is saved, there's no such pocket of time.

My initial suggestion is to try manipulating the data when SSWindowClosing is fired on the window, as this will occur before both winData is cleared, but also before the window data is saved to _closedWindows.

Would that event satisfy your needs? Or is there something more special that is required here?
Flags: needinfo?(morac99-firefox2)
(Reporter)

Comment 5

a year ago
Using SSWindowClosing should work as long as the current window state is updated prior to sending that notification.  

Looking at the code, though it doesn't look like that's the case.  It looks like the latest window state isn't grabbed until line 1242, which is after the SSWindowClosing event fires.

http://mxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/SessionStore.jsm#1219
Flags: needinfo?(morac99-firefox2)
(In reply to Michael Kraft [:morac] from comment #5)
> Using SSWindowClosing should work as long as the current window state is
> updated prior to sending that notification.  
> 
> Looking at the code, though it doesn't look like that's the case.  It looks
> like the latest window state isn't grabbed until line 1242, which is after
> the SSWindowClosing event fires.
> 
> http://mxr.mozilla.org/mozilla-central/source/browser/components/
> sessionstore/SessionStore.jsm#1219

Okay, gotcha. Can you give me more detail on what you're trying to do? Are you attempting to access the final data for the closed window once it's closed and gone for good? Or are you trying to catch it in a particular state before it's finally closed?
Flags: needinfo?(morac99-firefox2)
(Reporter)

Comment 7

a year ago
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #6) 
> Okay, gotcha. Can you give me more detail on what you're trying to do? Are
> you attempting to access the final data for the closed window once it's
> closed and gone for good? Or are you trying to catch it in a particular
> state before it's finally closed?

I'm trying to get its final state right before it closes (or at least since it has last changed).
Flags: needinfo?(morac99-firefox2)
(In reply to Michael Kraft [:morac] from comment #7)
> (In reply to Mike Conley (:mconley) - Needinfo me! from comment #6) 
> > Okay, gotcha. Can you give me more detail on what you're trying to do? Are
> > you attempting to access the final data for the closed window once it's
> > closed and gone for good? Or are you trying to catch it in a particular
> > state before it's finally closed?
> 
> I'm trying to get its final state right before it closes (or at least since
> it has last changed).

But strictly _before_ it has reached the closedWindows dict available via SessionStore.getClosedWindowData()?

Or is it alright to get at the last entry of the closedWindows dict after you know the window has finally closed and gone away?

If so, what you could try to do is, once you see the unload event on the window, call TabStateFlusher.flushWindow(window)[1], which will return a Promise. When that Promise resolves, then the last entry in the getClosedWindowData dict _should_ be the window that just closed.

We might need to add a new event to make this less hacky, but as a workaround, does that get you what you need?

[1]: TabStateFlusher can be imported via Cu.import("resource:///modules/sessionstore/TabStateFlusher.jsm");
Flags: needinfo?(morac99-firefox2)
(Reporter)

Comment 9

a year ago
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #8)
> (In reply to Michael Kraft [:morac] from comment #7)
> > (In reply to Mike Conley (:mconley) - Needinfo me! from comment #6) 
> > > Okay, gotcha. Can you give me more detail on what you're trying to do? Are
> > > you attempting to access the final data for the closed window once it's
> > > closed and gone for good? Or are you trying to catch it in a particular
> > > state before it's finally closed?
> > 
> > I'm trying to get its final state right before it closes (or at least since
> > it has last changed).
> 
> But strictly _before_ it has reached the closedWindows dict available via
> SessionStore.getClosedWindowData()?
> 
> Or is it alright to get at the last entry of the closedWindows dict after
> you know the window has finally closed and gone away?
> 
> If so, what you could try to do is, once you see the unload event on the
> window, call TabStateFlusher.flushWindow(window)[1], which will return a
> Promise. When that Promise resolves, then the last entry in the
> getClosedWindowData dict _should_ be the window that just closed.
> 
> We might need to add a new event to make this less hacky, but as a
> workaround, does that get you what you need?
> 
> [1]: TabStateFlusher can be imported via
> Cu.import("resource:///modules/sessionstore/TabStateFlusher.jsm");

I'm trying to get the window in its open state, though I suppose I can grab its closed state and then manipulate the data to make it appear as if it was open at that time.
Flags: needinfo?(morac99-firefox2)
(In reply to Michael Kraft [:morac] from comment #9)
> (In reply to Mike Conley (:mconley) - Needinfo me! from comment #8)
> > (In reply to Michael Kraft [:morac] from comment #7)
> > > (In reply to Mike Conley (:mconley) - Needinfo me! from comment #6) 
> > > > Okay, gotcha. Can you give me more detail on what you're trying to do? Are
> > > > you attempting to access the final data for the closed window once it's
> > > > closed and gone for good? Or are you trying to catch it in a particular
> > > > state before it's finally closed?
> > > 
> > > I'm trying to get its final state right before it closes (or at least since
> > > it has last changed).
> > 
> > But strictly _before_ it has reached the closedWindows dict available via
> > SessionStore.getClosedWindowData()?
> > 
> > Or is it alright to get at the last entry of the closedWindows dict after
> > you know the window has finally closed and gone away?
> > 
> > If so, what you could try to do is, once you see the unload event on the
> > window, call TabStateFlusher.flushWindow(window)[1], which will return a
> > Promise. When that Promise resolves, then the last entry in the
> > getClosedWindowData dict _should_ be the window that just closed.
> > 
> > We might need to add a new event to make this less hacky, but as a
> > workaround, does that get you what you need?
> > 
> > [1]: TabStateFlusher can be imported via
> > Cu.import("resource:///modules/sessionstore/TabStateFlusher.jsm");
> 
> I'm trying to get the window in its open state, though I suppose I can grab
> its closed state and then manipulate the data to make it appear as if it was
> open at that time.

Are you able to listen to the beforeunload event?
Flags: needinfo?(morac99-firefox2)
(Reporter)

Comment 11

a year ago
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #10)
> Are you able to listen to the beforeunload event?

It does not appear that the beforeunload event fires on chrome windows.  At least I'm never getting notifications for it.
Flags: needinfo?(morac99-firefox2)
(Reporter)

Comment 12

a year ago
Listening to "domwindowclosed" and grabbing a copy of the window state on that event and then using that state on the unload event seems to do what I want from limited testing.

That's not ideal since it's possible that the window data may be out of date though.
(In reply to Michael Kraft [:morac] from comment #12)
> Listening to "domwindowclosed" and grabbing a copy of the window state on
> that event and then using that state on the unload event seems to do what I
> want from limited testing.
> 
> That's not ideal since it's possible that the window data may be out of date
> though.

Yeah, I agree that's not ideal. I wonder if your best bet is to wait until the final collection has occurred, and then do the manipulation to make the window appear open, as you suggest in comment 9. I'm suggesting that instead of a new event after the collection here: http://mxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/SessionStore.jsm#1242 since we cannot guarantee that tabs are fully up to date at this point either - it's only after the flush has completed (and the window has been removed from the _windows dict) that we can make any data stability guarantees.

In order to wait until that final collection has occurred, I suggest using TabStateFlusher to flush the window in your domwindowclosed handler, and wait until that Promise is resolved, before accessing the closed window data.
Component: Session Restore → Extension Compatibility
Untracking, since this essentially sounds like a deliberate change that this particular addon will need to work-around / perform this action in a different way. If a product change is needed, please change back.
status-firefox46: affected → wontfix
status-firefox47: affected → wontfix
status-firefox48: affected → wontfix
status-firefox49: --- → affected
status-firefox50: --- → affected
status-firefox51: --- → affected
Dolske should this be WONTFIXed until/if we change plans here?
Flags: needinfo?(dolske)
Yes. My reading of the bug (notably comment 13) is that this issue is only triggered with an addon, and that addon needs to implement that workaround.
Status: NEW → RESOLVED
Last Resolved: a year ago
Flags: needinfo?(dolske)
Resolution: --- → WONTFIX
status-firefox49: affected → wontfix
status-firefox50: affected → wontfix
status-firefox51: affected → wontfix
You need to log in before you can comment on or make changes to this bug.