1424090 - Fix browser/components/sessionstore/test/{browser_forget_async_closings,browser_async_window_flushing}.js to run at expected timing after bug 1193394

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect)

Description

[Tooru Fujisawa [:arai]]

separated from bug 1421113.

browser/components/sessionstore/test/browser_forget_async_closings.js and browser/components/sessionstore/test/browser_async_window_flushing.js calls ss.getClosedWindowCount() etc just after the domwindowclosed notification, but at that point the window might not be in ss._closedWindows.
they should wait for the next event tick to make sure SessionStore finishes handling the window close.

Comment 1

[Tooru Fujisawa [:arai]]

looks like the issue is a bit more complicated.
the testcase expects the function resumes after onClose, but before the flush.
but with updated promise handling, the function resumes inside onClose.

Comment 2

[Tooru Fujisawa [:arai]]

So, the promise resolution handler gets called inside TabState.collect
https://searchfox.org/mozilla-central/rev/f5f1c3f294f89cfd242c3af9eb2c40d19d5e04e7/browser/components/sessionstore/TabState.jsm#90
>  _collectBaseTabData(tab, options) {
> ...

apparently XBL again.


I guess, with updated Promise handling, I think there's almost no way to run test in the expected timing.
now I'm if I can change the testcase to expect more stable state.

Comment 3

[Tooru Fujisawa [:arai]]

browser/components/sessionstore/SessionStore.jsm

> var SessionStoreInternal = {
> ...
>   observe: function ssi_observe(aSubject, aTopic, aData) {
>     switch (aTopic) {
> ...
>       case "domwindowclosed": // catch closed windows
>         this.onClose(aSubject).then(() => {

testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm

> this.BrowserTestUtils = {
> ...
>   domWindowClosed(win) {
>     return new Promise((resolve) => {
>       function observer(subject, topic, data) {
>         if (topic == "domwindowclosed" && (!win || subject === win)) {
>           Services.ww.unregisterNotification(observer);
>           resolve(subject.QueryInterface(Ci.nsIDOMWindow));
>         }
>       }
>       Services.ww.registerNotification(observer);
>     });
>   },

browser/components/sessionstore/test/browser_async_window_flushing.js

> add_task(async function test_remove_uninteresting_window() {
> ...
>   let domWindowClosed = BrowserTestUtils.domWindowClosed(newWin);
> ...
>   await domWindowClosed;
>   // OnClose has just finished running.
>   let currentClosedWindows = ss.getClosedWindowCount();

domWindowClosed promise gets resolved directly inside domwindowclosed notification, that may happen before ssi_observe.
in that case, the resolution hander will be executed in the next MicroTask checkpoint.
while executing ssi_observe, onClose may contain MicroTask checkpoint,
and in that case onClose haven't finished running when the test resumes after await.

then, there's no way to run resolution handler just after onClose finishes, at least with Promise.

Comment 4

[Tooru Fujisawa [:arai]]

at least about browser_async_window_flushing.js, it's just checking the intermediate state about the ss.getClosedWindowCount().
so I think there's another way to check what happened, even after flush happened.

Comment 5

[Tooru Fujisawa [:arai]]

if flush is guaranteed to take at least 2 event ticks, waiting for 1 event tick there may solve, but so far it doesn't take 2 event ticks locally.
now waiting for 1 event tick there *fixes*, but I wonder if it's just race...

Comment 6

[Tooru Fujisawa [:arai]]

Attachment: Wait for a event tick after domwindowclosed to make sure SessionStore.onClose finishes running in browser/components/sessionstore/test/{browser_async_window_flushing,browser_forget_async_closings}.js

So far, I put `await TestUtils.waitForTick()` after each `await domWindowClosed;` to make sure onClose finishes running.
Do you think it's safe to expect `TabStateFlusher.flushWindow(aWindow)` promise resolution handler gets executed after that?
(so far it does locally, both before and after bug 1193394)

Comment 7

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I'm finding this bug pretty hard to follow. I have a couple questions.

1. Suppose that the "domwindowclosed" notification happens and the SessionStoreInternal.onClose code runs first. That code will send a "SessionStore:flush" message to the child. In theory, the child could respond very quickly (with a "SessionStore:update" message) and the response message could be added to the event queue before we even run the test's `await domWindowClosed` and `await TestUtils.waitForTick()` code. In that case, by the time waitForTick() resolves, we'll have already processes the "SessionStore:update" message and so the test's getClosedWindowCount() assertion will fail. Have I made a mistake here?

2. I'm curious how you came to work on this code. Was the test failing with bug 1193394 without these changes? I don't understand why it would. Your changes only make the assertion run later, which seems like it makes it more likely to fail. The only purpose of the assertion is to ensure that we have a window that was being ignored until the update message arrived.

3. In comment 3, you said "apparently XBL again." How does XBL cause a promise resolution handler to run?

Overall, my recommendation here is to add some testing code to onClose that calls notifyObservers when onClose is done. Then you could add some code to the test that listens for that observer. That way you're guaranteed that the code will run exactly when you want it to.

Comment 8

[Tooru Fujisawa [:arai]]

thanks
and sorry I wasn't clear enough.

(In reply to Bill McCloskey (:billm) from comment #7)
> I'm finding this bug pretty hard to follow. I have a couple questions.
> 
> 1. Suppose that the "domwindowclosed" notification happens and the
> SessionStoreInternal.onClose code runs first. That code will send a
> "SessionStore:flush" message to the child. In theory, the child could
> respond very quickly (with a "SessionStore:update" message) and the response
> message could be added to the event queue before we even run the test's
> `await domWindowClosed` and `await TestUtils.waitForTick()` code. In that
> case, by the time waitForTick() resolves, we'll have already processes the
> "SessionStore:update" message and so the test's getClosedWindowCount()
> assertion will fail. Have I made a mistake here?

Yes, the patch tries to delay the execution for the reason mentioned in the answer for 2, 3 below.
if delaying it like that is bad, I think we should look for completely different way to test it.
(since we cannot execute the test in expected timing)


> 2. I'm curious how you came to work on this code. Was the test failing with
> bug 1193394 without these changes? I don't understand why it would. Your
> changes only make the assertion run later, which seems like it makes it more
> likely to fail. The only purpose of the assertion is to ensure that we have
> a window that was being ignored until the update message arrived.

This test doesn't fail without bug 1193394 change.
this fails after bug 1193394 because the test resumes from `await domWindowClosed;` *inside* onClose, before onClose finishes running.
that means the _closedWindows is not yet updated, and the assert about the number of closed windows fails.
so, without fixing the test, after bug 1193394, the test checks the number of closed windows before onClose modified it.


> 3. In comment 3, you said "apparently XBL again." How does XBL cause a
> promise resolution handler to run?

The promise resolution handler is executed in MicroTask checkpoint (see bevis's slide in bug 1193394 comment #47),
and XBL accessor contains MicroTask checkpoint (see bug 1416153 comment #4 for more details).
So, if onClose contains the access to XBL accessor, already-resolved promise's resolution handler will be executed there.
then, apparently domwindowclosed observer for BrowserTestUtils.domWindowClosed is executed before SessionStore's one,
and the promise is already resolved when onClose is called.


> Overall, my recommendation here is to add some testing code to onClose that
> calls notifyObservers when onClose is done. Then you could add some code to
> the test that listens for that observer. That way you're guaranteed that the
> code will run exactly when you want it to.

do you mean that adding dedicated observer notification in SessionStore for testing?

Comment 9

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to Tooru Fujisawa [:arai] from comment #8)
> Yes, the patch tries to delay the execution for the reason mentioned in the
> answer for 2, 3 below.
> if delaying it like that is bad, I think we should look for completely
> different way to test it.
> (since we cannot execute the test in expected timing)

OK, I agree.

> This test doesn't fail without bug 1193394 change.
> this fails after bug 1193394 because the test resumes from `await
> domWindowClosed;` *inside* onClose, before onClose finishes running.
> that means the _closedWindows is not yet updated, and the assert about the
> number of closed windows fails.
> so, without fixing the test, after bug 1193394, the test checks the number
> of closed windows before onClose modified it.

When does onClose modify _closedWindows? The reason I'm confused is that the "number of closed windows" assertion should be true even before the `await domWindowClosed;` line (and I just checked that it is). I guess maybe onClose is temporarily doing something that makes it false, but then it becomes true again?

> > 3. In comment 3, you said "apparently XBL again." How does XBL cause a
> > promise resolution handler to run?
> 
> The promise resolution handler is executed in MicroTask checkpoint (see
> bevis's slide in bug 1193394 comment #47),
> and XBL accessor contains MicroTask checkpoint (see bug 1416153 comment #4
> for more details).
> So, if onClose contains the access to XBL accessor, already-resolved
> promise's resolution handler will be executed there.
> then, apparently domwindowclosed observer for
> BrowserTestUtils.domWindowClosed is executed before SessionStore's one,
> and the promise is already resolved when onClose is called.

OK. It seems really weird to me that we have nsAutoMicroTask here:
https://searchfox.org/mozilla-central/rev/fd0b701b6f4bce8dead150c569dd68fb9f4c7ed9/dom/xbl/nsXBLProtoImplField.cpp#390
It makes it really hard to reason about where microtasks will occur. In JS-land, people tend to think of XBL getters/setters just like functions. It's very odd that a microtask would be invoked here. Olli, why do you think it should work this way?

> > Overall, my recommendation here is to add some testing code to onClose that
> > calls notifyObservers when onClose is done. Then you could add some code to
> > the test that listens for that observer. That way you're guaranteed that the
> > code will run exactly when you want it to.
> 
> do you mean that adding dedicated observer notification in SessionStore for
> testing?

Yes. I think there's some precedent for this. See the browser.sessionstore.debug preference:
https://searchfox.org/mozilla-central/rev/fd0b701b6f4bce8dead150c569dd68fb9f4c7ed9/browser/components/sessionstore/SessionStore.jsm#976

Comment 10

[Tooru Fujisawa [:arai]]

(In reply to Bill McCloskey (:billm) from comment #9)
> When does onClose modify _closedWindows?

here:
https://searchfox.org/mozilla-central/rev/fd0b701b6f4bce8dead150c569dd68fb9f4c7ed9/browser/components/sessionstore/SessionStore.jsm#1570
>  maybeSaveClosedWindow(winData, isLastWindow) {
> ...
>         this._closedWindows.splice(index, 0, winData);

https://searchfox.org/mozilla-central/rev/fd0b701b6f4bce8dead150c569dd68fb9f4c7ed9/browser/components/sessionstore/SessionStore.jsm#1453
>         this.maybeSaveClosedWindow(winData, isLastWindow);
> ...
>         this.maybeSaveClosedWindow(winData, isLastWindow);

and the assertion in the test checks that splice already happened.

https://searchfox.org/mozilla-central/rev/fd0b701b6f4bce8dead150c569dd68fb9f4c7ed9/browser/components/sessionstore/test/browser_async_window_flushing.js#129
>   is(currentClosedWindows, initialClosedWindows + 1,


> The reason I'm confused is that the
> "number of closed windows" assertion should be true even before the `await
> domWindowClosed;` line (and I just checked that it is). I guess maybe
> onClose is temporarily doing something that makes it false, but then it
> becomes true again?

onClose makes the first assertion true (closed window's count is initial+1), and flush's resolution handler makes the second assertion true (closed window's count equals to initial).

1st
https://searchfox.org/mozilla-central/rev/fd0b701b6f4bce8dead150c569dd68fb9f4c7ed9/browser/components/sessionstore/test/browser_async_window_flushing.js#129
>   is(currentClosedWindows, initialClosedWindows + 1,

2nd
https://searchfox.org/mozilla-central/rev/fd0b701b6f4bce8dead150c569dd68fb9f4c7ed9/browser/components/sessionstore/test/browser_async_window_flushing.js#135-136
>   is(currentClosedWindows,
>      initialClosedWindows,

so, the issue here is the 1st assertion, that becomes true only after onClose, before the flush's resolution handler.


> Yes. I think there's some precedent for this. See the
> browser.sessionstore.debug preference:
> https://searchfox.org/mozilla-central/rev/
> fd0b701b6f4bce8dead150c569dd68fb9f4c7ed9/browser/components/sessionstore/
> SessionStore.jsm#976

okay, I'll try that.

Comment 11

[Tooru Fujisawa [:arai]]

I meant
> https://searchfox.org/mozilla-central/rev/
> fd0b701b6f4bce8dead150c569dd68fb9f4c7ed9/browser/components/sessionstore/
> SessionStore.jsm#1453
> >  onClose: function ssi_onClose(aWindow) {
> > ...
> >         this.maybeSaveClosedWindow(winData, isLastWindow);

Comment 12

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Ah, I see why I was confused. I was looking at the first test in that file (test_add_interesting_window), not the second one (test_remove_uninteresting_window). That makes more sense. Thank you.

Comment 13

[Tooru Fujisawa [:arai]]

Attachment: Use dedicated observer notification instead of domwindowclosed to catch onClosed in testcases for SessionStore.jsm.

Added sessionstore-debug-domwindowclosed-handled observer notification and used it in tests.

Comment 14

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8935968 [details] [diff] [review]
Use dedicated observer notification instead of domwindowclosed to catch onClosed in testcases for SessionStore.jsm.

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

Thanks!

Comment 15

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8f36094d67c5faf0d91a258b6dcb51d7fc8fdd94
Bug 1424090 - Use dedicated observer notification instead of domwindowclosed to catch onClosed in testcases for SessionStore.jsm. r=billm

Comment 16

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/8f36094d67c5

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

MicroTask handling and XBL is a bit unclear yes, but to me XBL getters/setters should have similar microtask handling as what for example CustomElement reactions have.