Closed Bug 1424090 Opened 2 years ago Closed 2 years ago

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

Categories

(Firefox :: Session Restore, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 59
Tracking Status
firefox59 --- fixed

People

(Reporter: arai, Assigned: arai)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
Summary: browser/components/sessionstore/test/{browser_forget_async_closings,browser_async_window_flushing}.js should wait for the next event tick after the window gets closed → Fix browser/components/sessionstore/test/{browser_forget_async_closings,browser_async_window_flushing}.js to run at expected timing after bug 1193394
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.
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.
Component: General → Session Restore
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.
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...
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)
Attachment #8935581 - Flags: review?(wmccloskey)
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.
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?
(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
Flags: needinfo?(bugs)
(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.
I meant
> https://searchfox.org/mozilla-central/rev/
> fd0b701b6f4bce8dead150c569dd68fb9f4c7ed9/browser/components/sessionstore/
> SessionStore.jsm#1453
> >  onClose: function ssi_onClose(aWindow) {
> > ...
> >         this.maybeSaveClosedWindow(winData, isLastWindow);
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.
Added sessionstore-debug-domwindowclosed-handled observer notification and used it in tests.
Attachment #8935581 - Attachment is obsolete: true
Attachment #8935581 - Flags: review?(wmccloskey)
Attachment #8935968 - Flags: review?(wmccloskey)
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!
Attachment #8935968 - Flags: review?(wmccloskey) → review+
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
https://hg.mozilla.org/mozilla-central/rev/8f36094d67c5
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
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.
Flags: needinfo?(bugs)
You need to log in before you can comment on or make changes to this bug.