897095 - "System JS : ERROR resource:///modules/sessionstore/SessionStore.jsm:4307 \n TypeError: this._windows[aWindow.__SSi] is undefined" on green debug test runs

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect)

Description

[Ed Morley [:emorley]]

A soon to be deployed TBPL parser improvement has found a whole bunch of log spam in green debug mochitest-browser-chrome runs.

a) Should these be making the test run fail?
b) We should either null check or fix the root cause if this is unexpected, so we don't spam the annotated failure summary once this TBPL patchset is rolled out.

eg:

https://tbpl-dev.allizom.org/php/getParsedLog.php?id=25604620&tree=Mozilla-Central#error2

Rev4 MacOSX Lion 10.7 mozilla-central debug test mochitest-browser-chrome on 2013-07-23 02:32:38 PDT for push fb4bf993a58a

slave: talos-r4-lion-044

{
02:58:05     INFO -  INFO TEST-END | chrome://mochitests/content/browser/browser/components/sessionstore/test/browser_491577.js | finished in 1315ms
02:58:05     INFO -  TEST-INFO | checking window state
02:58:05     INFO -  TEST-INFO | chrome://mochitests/content/browser/browser/components/sessionstore/test/browser_491577.js | must wait for focus
02:58:05     INFO -  WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80040111: file ../../../../content/events/src/nsContentEventHandler.cpp, line 93
02:58:06     INFO -  TEST-START | chrome://mochitests/content/browser/browser/components/sessionstore/test/browser_495495.js
02:58:06     INFO -  ++DOCSHELL 0x1624c8f30 == 88 [id = 1945]
02:58:06     INFO -  ++DOMWINDOW == 210 (0x15ee1f1c8) [serial = 5230] [outer = 0x0]
02:58:06     INFO -  ++DOMWINDOW == 211 (0x164726f48) [serial = 5231] [outer = 0x15ee1f1c8]
02:58:06     INFO -  WARNING: NS_ENSURE_TRUE(mTextInputHandler) failed: file ../../../widget/cocoa/nsChildView.mm, line 5113
02:58:06     INFO -  System JS : ERROR resource:///modules/sessionstore/SessionStore.jsm:4307
02:58:06     INFO -                       TypeError: this._windows[aWindow.__SSi] is undefined
}

http://mxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/src/SessionStore.jsm#4370

4362    /**
4363    * Set the given window's busy state
4364    * @param aWindow the window
4365    * @param aValue the window's busy state
4366    */
4367   _setWindowStateBusyValue:
4368     function ssi_changeWindowStateBusyValue(aWindow, aValue) {
4369 
4370     this._windows[aWindow.__SSi].busy = aValue;
4371 
4372     // Keep the to-be-restored state in sync because that is returned by
4373     // getWindowState() as long as the window isn't loaded, yet.
4374     if (!this._isWindowLoaded(aWindow)) {
4375       let stateToRestore = this._statesToRestore[aWindow.__SS_restoreID].windows[0];
4376       stateToRestore.busy = aValue;
4377     }
4378   },

Comment 1

[David Teller [:Yoric] (please use "needinfo")]

I am not familiar with that bit of the code, but yes, it looks like these should cause failures.

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

I assume those are caused by _setWindowStateReady() calls done off a timeout after the window has been closed. We should definitely fix that.

Comment 3

[Ryan VanderMeulen [:RyanVM]]

(In reply to Tim Taubert [:ttaubert] from comment #2)
> I assume those are caused by _setWindowStateReady() calls done off a timeout
> after the window has been closed. We should definitely fix that.

I'm going to assume you're OK with being assigned to this then :)

Comment 4

[Ed Morley [:emorley]]

(In reply to Tim Taubert [:ttaubert] from comment #2)
> I assume those are caused by _setWindowStateReady() calls done off a timeout
> after the window has been closed. We should definitely fix that.

:-)

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

Sorry I don't have time to investigate this right now. I took a stab at it a couple of days ago but it wasn't as easy as I assumed in comment #2. Should not be too hard to fix once we identified the actual cause.

Comment 6

[Steven MacLeod [:smacleod]]

I'm having trouble reproducing this locally, or on Try. Tim, were you able to?

Comment 7

[Bobby Holley (:bholley)]

bug 916945 might make this show up more.

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

I currently have no idea what's causing this. I pushed a tiny patch to try that throws when we hit this line of code with this._windows[aWindow.__SSi] being undefined.

https://tbpl.mozilla.org/?tree=Try&rev=ae19bdf67e3c

Comment 9

[Tim Taubert [:ttaubert] (inactive)]

Ok the try push didn't help much because I forgot to dump the stack trace. I thought the test harness does that, oh well :) At least I found an occurrence of the message that I can reproduce locally by running browser_637020.js.

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

It's caused by exactly what I said in comment #2. Not sure why my first attempt wasn't successful. Anyway, easy to fix.

Comment 11

[Tim Taubert [:ttaubert] (inactive)]

Attachment: Don't call restoreHistory() for closed windows

https://tbpl.mozilla.org/?tree=Try&rev=d4a7ec669fd9

Comment 12

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Tim Taubert [:ttaubert] from comment #11)
> https://tbpl.mozilla.org/?tree=Try&rev=d4a7ec669fd9

I checked all m-bc logs of the above try changeset and I didn't see a single "this._windows[aWindow.__SSi] is undefined" error message.

Comment 13

[Ed Morley [:emorley]]

Thank you for fixing this! :-)

Comment 14

[Steven MacLeod [:smacleod]]

Comment on attachment 808607 [details] [diff] [review]
Don't call restoreHistory() for closed windows

Review of attachment 808607 [details] [diff] [review]:
-----------------------------------------------------------------

Looks all good to me!

Comment 15

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/integration/fx-team/rev/7a900d222edd

Comment 16

[Wes Kocher (:KWierso)]

https://hg.mozilla.org/mozilla-central/rev/7a900d222edd

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Tim, can you please request Aurora approval for this please?

Comment 18

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 808607 [details] [diff] [review]
Don't call restoreHistory() for closed windows

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Has been around for a while.
User impact if declined: None.
Testing completed (on m-c, etc.): Landed on m-c without issues.
Risk to taking this patch (and alternatives if risky): Very low-risk change.
String or IDL/UUID changes made by this patch: None.

We'd like to uplift this mostly to get rid of a couple of failures when running the test suites. It's quite unlikely that this is actually hit in the wild but it's also a very safe fix.

Comment 19

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 808607 [details] [diff] [review]
Don't call restoreHistory() for closed windows

always a fan of low risk uplifts that help remove test failures, approving.

Comment 20

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/bb3cf62469e6