Closed Bug 1167579 Opened 4 years ago Closed 4 years ago

Intermittent browser_broadcast.js,browser_sessionStorage.js | Uncaught exception - at chrome://mochitests/content/browser/browser/components/sessionstore/test/browser_broadcast.js:60 - TypeError: storage is undefined

Categories

(Firefox :: Session Restore, defect)

defect
Not set
Points:
2

Tracking

()

RESOLVED FIXED
Firefox 41
Iteration:
41.1 - May 25
Tracking Status
firefox39 --- unaffected
firefox40 --- fixed
firefox41 --- fixed
firefox-esr31 --- unaffected
firefox-esr38 --- unaffected

People

(Reporter: RyanVM, Assigned: ttaubert)

References

Details

(Keywords: intermittent-failure)

Attachments

(2 files, 1 obsolete file)

+++ This bug was initially created as a clone of Bug #1147396 +++

03:48:29 INFO - *************************
03:48:29 INFO - A coding exception was thrown and uncaught in a Task.
03:48:29 INFO - Full message: TypeError: storage is undefined
03:48:29 INFO - Full stack: flush_on_duplicate@chrome://mochitests/content/browser/browser/components/sessionstore/test/browser_broadcast.js:60:3
03:48:29 INFO - Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:867:23
03:48:29 INFO - this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:746:7
03:48:29 INFO - this.PromiseWalker.scheduleWalkerLoop/<@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:688:37
03:48:29 INFO - Promise*this.PromiseWalker.scheduleWalkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:688:5
03:48:29 INFO - this.PromiseWalker.schedulePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:709:7
03:48:29 INFO - this.PromiseWalker.completePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:671:7
03:48:29 INFO - listener@chrome://mochitests/content/browser/browser/components/sessionstore/test/head.js:197:7
03:48:29 INFO - *************************
03:48:29 INFO - 1644 INFO checking window state
03:48:29 INFO - 1645 INFO Entering test flush_on_tabclose
03:48:29 INFO - 1646 INFO TEST-PASS | browser/components/sessionstore/test/browser_broadcast.js | sessionStorage data has been flushed on TabClose
03:48:29 INFO - 1647 INFO Leaving test flush_on_tabclose
03:48:29 INFO - 1648 INFO Entering test flush_on_quit_requested
03:48:29 INFO - 1649 INFO TEST-PASS | browser/components/sessionstore/test/browser_broadcast.js | sessionStorage data has been flushed when a quit is requested
03:48:29 INFO - 1650 INFO Leaving test flush_on_quit_requested
03:48:29 INFO - 1651 INFO Entering test flush_on_duplicate
03:48:29 INFO - 1652 INFO TEST-UNEXPECTED-FAIL | browser/components/sessionstore/test/browser_broadcast.js | Uncaught exception - at chrome://mochitests/content/browser/browser/components/sessionstore/test/browser_broadcast.js:60 - TypeError: storage is undefined
03:48:29 INFO - Stack trace:
03:48:29 INFO - flush_on_duplicate@chrome://mochitests/content/browser/browser/components/sessionstore/test/browser_broadcast.js:60:3
03:48:29 INFO - Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:867:23
03:48:29 INFO - this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:746:7
03:48:29 INFO - this.PromiseWalker.scheduleWalkerLoop/<@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:688:37
03:48:29 INFO - Promise*this.PromiseWalker.scheduleWalkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:688:5
03:48:29 INFO - this.PromiseWalker.schedulePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:709:7
03:48:29 INFO - this.PromiseWalker.completePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:671:7
03:48:29 INFO - listener@chrome://mochitests/content/browser/browser/components/sessionstore/test/head.js:197:7
03:48:29 INFO - Tester_execTest@chrome://mochikit/content/browser-test.js:741:9
03:48:29 INFO - Tester.prototype.nextTest</<@chrome://mochikit/content/browser-test.js:664:7
03:48:29 INFO - SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:745:59
03:48:29 INFO - 1653 INFO Leaving test flush_on_duplicate
03:48:29 INFO - 1654 INFO Entering test flush_on_windowclose
Tim, maybe related to your recent sessionstore test changes?
Flags: needinfo?(ttaubert)
Assignee: nobody → ttaubert
Status: NEW → ASSIGNED
Flags: needinfo?(ttaubert)
Attachment #8609700 - Flags: review?(wmccloskey)
Blocks: 1010411
Blocks: 1140375
Iteration: --- → 41.1 - May 25
Flags: qe-verify?
Flags: firefox-backlog+
Points: --- → 2
Comment on attachment 8609700 [details] [diff] [review]
0001-Bug-1167579-Fix-intermittent-browser_broadcast.js-fa.patch

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

::: browser/components/sessionstore/test/browser_sessionStorage.js
@@ +29,5 @@
>    is(storage["http://mochi.test:8888"].test, OUTER_VALUE,
>      "sessionStorage data for mochi.test has been serialized correctly");
>  
>    // Ensure that modifying sessionStore values works for the inner frame only.
> +  yield modifySessionStorage(browser, {test: "modified1"}, 0);

It's very easy to miss the meaning of the 0 here. Could you maybe pass an options parameter with {frameIndex: 0}?

::: browser/components/sessionstore/test/head.js
@@ +656,5 @@
> +          resolve();
> +        }
> +      }, true);
> +
> +      for (let key of [...keys]) {

Can't you just say |for (let key of keys)|?
Attachment #8609700 - Flags: review?(wmccloskey) → review+
(In reply to Bill McCloskey (:billm) from comment #26)
> >    // Ensure that modifying sessionStore values works for the inner frame only.
> > +  yield modifySessionStorage(browser, {test: "modified1"}, 0);
> 
> It's very easy to miss the meaning of the 0 here. Could you maybe pass an
> options parameter with {frameIndex: 0}?

Sure, will do.

> > +      for (let key of [...keys]) {
> 
> Can't you just say |for (let key of keys)|?

Did this to account for sync changes that might modify the set while we're iterating but as MozStorageChanged events are always dispatched off a runnable I guess we don't need this.
https://hg.mozilla.org/mozilla-central/rev/994744c57cbc
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
Summary: Intermittent browser_broadcast.js | Uncaught exception - at chrome://mochitests/content/browser/browser/components/sessionstore/test/browser_broadcast.js:60 - TypeError: storage is undefined | Found an unexpected tab at the end of test run → Intermittent browser_broadcast.js,browser_sessionStorage.js | Uncaught exception - at chrome://mochitests/content/browser/browser/components/sessionstore/test/browser_broadcast.js:60 - TypeError: storage is undefined
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: Firefox 41 → ---
Can reproduce locally, will have a fix soon.
Flags: qe-verify? → qe-verify-
OS: Mac OS X → All
Hardware: x86_64 → All
This is a fix for only browser_broadcast.js. We currently duplicate the tab and then call getTabState() without flushing. I'm actually surprised this doesn't fail more frequently. The easy fix here is to remove the superfluous check and rely only on checking the closedTabData. If we properly flushed when duplicating the tab the data will be in there.
Attachment #8609700 - Attachment is obsolete: true
Attachment #8612615 - Flags: review?(wmccloskey)
browser_sessionStorage.js seems to sometimes fail because Date.now() isn't monotonous, that occurs esp. often on Win32 machines. The closedAt property of the closed tab might be "wrong" and then the closed tab is put into the wrong spot and we check the wrong closedTabData.
Attachment #8612979 - Flags: review?(wmccloskey)
Attachment #8612615 - Flags: review?(wmccloskey) → review+
Comment on attachment 8612979 [details] [diff] [review]
0002-Bug-1167579-Fix-intermittent-browser_sessionStorage..patch

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

I guess this is fine, but it kinda suggests we might be better off using a global counter rather than Date.now().

::: browser/components/sessionstore/test/browser_sessionStorage.js
@@ +164,5 @@
>    [{state: {storage}}] = JSON.parse(ss.getClosedTabData(window));
>    ok(!storage, "sessionStorage data has *not* been saved");
>  
> +  // Remove all closed tabs before continuing with the next test.
> +  // As Date.now() isn't monotonous we might sometimes check

I think the word you want is "monotonic". Monotonous basically means boring :-).
Attachment #8612979 - Flags: review?(wmccloskey) → review+
(In reply to Bill McCloskey (:billm) from comment #70)
> I guess this is fine, but it kinda suggests we might be better off using a
> global counter rather than Date.now().

Yeah we would be. closedAt was introduced to give UI or add-ons a chance to provide more information about closed tabs, it wasn't really meant to provide a sort order. Also, it should mostly not be a problem in everyday use - probably more a test problem.

> > +  // Remove all closed tabs before continuing with the next test.
> > +  // As Date.now() isn't monotonous we might sometimes check
> 
> I think the word you want is "monotonic". Monotonous basically means boring
> :-).

Hah, oops :)
(In reply to Tim Taubert [:ttaubert] from comment #78)
> (In reply to Bill McCloskey (:billm) from comment #70)
> > > +  // Remove all closed tabs before continuing with the next test.
> > > +  // As Date.now() isn't monotonous we might sometimes check
> > 
> > I think the word you want is "monotonic". Monotonous basically means boring
> > :-).
> 
> Hah, oops :)

Forgot to fix the commit message. Oh well.
https://hg.mozilla.org/mozilla-central/rev/e3af4ebf8ffb
https://hg.mozilla.org/mozilla-central/rev/cf4d1a0497a6
Status: REOPENED → RESOLVED
Closed: 4 years ago4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
You need to log in before you can comment on or make changes to this bug.