Closed Bug 897095 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox :: Session Restore, defect)

x86_64
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 27
Tracking Status
firefox25 --- unaffected
firefox26 --- fixed
firefox27 --- fixed
firefox-esr24 --- unaffected

People

(Reporter: emorley, Assigned: ttaubert)

References

(Blocks 1 open bug)

Details

(Whiteboard: [qa-])

Attachments

(1 file)

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   },
Flags: needinfo?(dteller)
I am not familiar with that bit of the code, but yes, it looks like these should cause failures.
Flags: needinfo?(dteller)
I assume those are caused by _setWindowStateReady() calls done off a timeout after the window has been closed. We should definitely fix that.
(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 :)
Assignee: nobody → ttaubert
(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.

:-)
Flags: needinfo?(ttaubert)
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.
Assignee: ttaubert → nobody
Flags: needinfo?(ttaubert)
Assignee: nobody → smacleod
Status: NEW → ASSIGNED
No longer blocks: 892958
Blocks: log-SnR
I'm having trouble reproducing this locally, or on Try. Tim, were you able to?
Flags: needinfo?(ttaubert)
bug 916945 might make this show up more.
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
Flags: needinfo?(ttaubert)
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.
It's caused by exactly what I said in comment #2. Not sure why my first attempt wasn't successful. Anyway, easy to fix.
(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.
Thank you for fixing this! :-)
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!
Attachment #808607 - Flags: review?(smacleod) → review+
Blocks: 920191
No longer blocks: log-SnR
https://hg.mozilla.org/mozilla-central/rev/7a900d222edd
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 27
Tim, can you please request Aurora approval for this please?
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.
Attachment #808607 - Flags: approval-mozilla-aurora?
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.
Attachment #808607 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.