separated from bug 1193394 comment #112 - bug 1193394 comment #118. the following tests fail after bug 1193394, because they expect the "closed tabs" information is updated just after closing tab, but it needs extra tick. browser/base/content/test/tabs/browser_bug580956.js browser/components/sessionstore/test/browser_350525.js browser/components/sessionstore/test/browser_454908.js browser/components/sessionstore/test/browser_456342.js browser/components/sessionstore/test/browser_628270.js browser/components/sessionstore/test/browser_911547.js browser/components/sessionstore/test/browser_broadcast.js browser/components/sessionstore/test/browser_dying_cache.js browser/components/sessionstore/test/browser_formdata.js browser/components/sessionstore/test/browser_formdata_cc.js browser/components/sessionstore/test/browser_frame_history.js browser/components/sessionstore/test/browser_page_title.js browser/components/sessionstore/test/browser_sessionHistory.js browser/base/content/test/general/browser_bug817947.js browser/base/content/test/general/browser_ctrlTab.js I think requiring an extra tick doesn't matter in practice (outside of automated tests), so I'll fix them to wait a bit.

Do we have non-test code that expects this information to update immediately?

try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7aea0a3224ba478bdfe3be603fae4f3bbf19e61d (In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #1) > Do we have non-test code that expects this information to update immediately? so far I don't see any test failure that needs non-test fix. in all cases the test touches the information just after removing tab.

basically, added TestUtils.waitForTick() everywhere, between closing tab and touching closed tabs info. also converted some callbacks into async function just to avoid extra indentation. at least they passed locally, try is running now.

Comment on attachment 8954806 [details] [diff] [review] Wait for an event tick after removing tab before using closed tabs information. >--- a/browser/base/content/test/general/browser_ctrlTab.js >+++ b/browser/base/content/test/general/browser_ctrlTab.js >+async function promiseRemoveTabAndSessionState(tabToClose) { >+ await BrowserTestUtils.removeTab(tabToClose); >+ >+ // Wait for an event tick to make sure session state is up to date. >+ await TestUtils.waitForTick(); >+} Afaik this test doesn't care about closed tab data so it's not really clear to me what you're doing here.

(In reply to Dão Gottwald [::dao] from comment #4) > Comment on attachment 8954806 [details] [diff] [review] > Wait for an event tick after removing tab before using closed tabs > information. > > >--- a/browser/base/content/test/general/browser_ctrlTab.js > >+++ b/browser/base/content/test/general/browser_ctrlTab.js > > >+async function promiseRemoveTabAndSessionState(tabToClose) { > >+ await BrowserTestUtils.removeTab(tabToClose); > >+ > >+ // Wait for an event tick to make sure session state is up to date. > >+ await TestUtils.waitForTick(); > >+} > > Afaik this test doesn't care about closed tab data so it's not really clear > to me what you're doing here. The test calls `undoCloseTab()` just after removing tab, and if we don't wait it operates `undo` on the state before removing the tab. https://searchfox.org/mozilla-central/rev/769222fadff46164f8cc0dc7a0bae5a60dc2f335/browser/base/content/test/general/browser_ctrlTab.js#64-66 (so... maybe we can just touch that single case tho...)

then, looks like I overlooked some other cases https://treeherder.mozilla.org/#/jobs?repo=try&revision=7aea0a3224ba478bdfe3be603fae4f3bbf19e61d&selectedJob=164889638 I'll post followup shortly.

fixed 3 more cases: * browser/components/sessionstore/test/browser_579879.js * browser/components/sessionstore/test/browser_async_remove_tab.js * browser/components/sessionstore/test/browser_sessionStorage.js

looks like the change in bug 888600 caused the issue.

(In reply to Tooru Fujisawa [:arai] from comment #8) > looks like the change in bug 888600 caused the issue. especially https://hg.mozilla.org/integration/mozilla-inbound/rev/af5303781961

Here the closed tabs information is updated, triggered by message. saveClosedTabData splice saveClosedTabData@resource:///modules/sessionstore/SessionStore.jsm:2070:81 receiveMessage@resource:///modules/sessionstore/SessionStore.jsm:908:15 MessageListener.receiveMessage*onLoad/<@resource:///modules/sessionstore/SessionStore.jsm:1098:7 onLoad@resource:///modules/sessionstore/SessionStore.jsm:1096:5 onBeforeBrowserWindowShown@resource:///modules/sessionstore/SessionStore.jsm:1281:5 ssi_observe@resource:///modules/sessionstore/SessionStore.jsm:766:9 onLoad@chrome://browser/content/browser.js:1321:5 onload@chrome://browser/content/browser.xul:1:1 https://searchfox.org/mozilla-central/rev/14d933246211b02f5be21d2e730a57cf087c6606/browser/components/sessionstore/SessionStore.jsm#2070 > saveClosedTabData(closedTabs, tabData) { > ... > // Insert tabData at the right position. > closedTabs.splice(index, 0, tabData); and indeed bug 888600 touched the message manager. So, I think it means that the current code relies on that some message arrives synchronously (or maybe async but soon), but as long as it's async message, the assumption was wrong, and fixing testcases that relies on it should be fixed.

peterv, can you confirm that bug 888600 patch is expected to change the message handler invocation timing?

(In reply to Tooru Fujisawa [:arai] from comment #10) > arrives synchronously (or maybe async but soon), > but as long as it's async message, the assumption was wrong, and fixing > testcases that relies on it should be fixed. should be *fine.

:hiro found the issue. Before bug 1193394, Promise resolution handler is executed at the end of Task, but bug 1193394 changes it to each MicroTask checkpoint. Then, bug 888600 changes nsFrameMessageManager::ReceiveMessage to use WebIDL, and that binding contains MicroTask checkpoint. So, already-resolved Promise's resolution handler will be executed there, earlier than expected. https://searchfox.org/mozilla-central/source/dom/bindings/CallbackObject.cpp#352-359 > CallbackObject::CallSetup::~CallSetup() > { > ... > // It is important that this is the last thing we do, after leaving the > // compartment and undoing all our entry/incumbent script changes > if (mIsMainThread) { > CycleCollectedJSContext* ccjs = CycleCollectedJSContext::Get(); > if (ccjs) { > ccjs->LeaveMicroTask(); > } > } > } __GENERATED__/dom/bindings/MessageManagerBinding.h > template <typename T> > inline void > ReceiveMessage(...) > { > ... > CallSetup s(this, aRv, aExecutionReason, aExceptionHandling, aCompartment); > if (!s.GetContext()) { > MOZ_ASSERT(aRv.Failed()); > return; > } > JS::Rooted<JS::Value> thisValJS(s.GetContext()); > if (!ToJSValue(s.GetContext(), thisVal, &thisValJS)) { > aRv.Throw(NS_ERROR_FAILURE); > return; > } > return ReceiveMessage(s.GetContext(), thisValJS, argument, aRetVal, aRv); > } https://searchfox.org/mozilla-central/rev/769222fadff46164f8cc0dc7a0bae5a60dc2f335/dom/base/nsFrameMessageManager.cpp#1181 > nsresult > nsFrameMessageManager::ReceiveMessage(nsISupports* aTarget, > nsIFrameLoader* aTargetFrameLoader, > bool aTargetClosed, > const nsAString& aMessage, > bool aIsSync, > StructuredCloneData* aCloneData, > mozilla::jsipc::CpowHolder* aCpows, > nsIPrincipal* aPrincipal, > nsTArray<StructuredCloneData>* aRetVal) > { > ... > webIDLListener->ReceiveMessage(thisValue, argument, &rval, rv); bz, is it expected outcome from the change? do you think we should just fix the testcase that depends on previous invocation timing?

CCing Olli and :kmag. I am bit concerned that if it's expected behavior, does it also affect extension APIs?

(In reply to Hiroyuki Ikezoe (:hiro) from comment #14) > CCing Olli and :kmag. > I am bit concerned that if it's expected behavior, does it also affect > extension APIs? It's unlikely to affect extension APIs. Extension APIs that deal with tab state are all async, and mostly cross-process. I suppose there might be some way to make it matter if you dispatched two API calls in the same tick, and tried really hard to find a way to make them rely on the old behavior... But even then I don't think you'd wind up with reliably good or bad behavior.

Thanks for the quick reply! So the biggest problem here is that the failed test cases rely on the process what has done in the message listener in browser code but also the test itself listen the message to proceed test. So if there are such code in non-test code as bz mentioned in comment 1, it won't work as expected.

> bz, is it expected outcome from the change? Somewhat. Per spec, when calling a WebIDL callback, a microtask checkpoint is done after the call, if the JS stack is empty. I don't know whether we get this last condition right; I was assuming that was one of the things bug 1193394 was addressing as needed. So when nsFrameMessageManager::ReceiveMessage is called, is there any JS on the stack? If not, the a microtask checkpoint right before webIDLListener->ReceiveMessage() returns is the right behavior. It's entirely possible that C++-to-JS calls via XPConnect don't do a microtask checkpoint on return even if the JS stack is empty. Arguably that's buggy and we should fix it...

CCing some other browser folks. Thanks bz! So as for this failure case, we should just wait for the next tick in BrowserTestUtils.tabRemoved, something like this; https://hg.mozilla.org/try/rev/76a01a4ad3cd3d9ae7aea6151e5079dce1dec028 let's see if what happens. https://treeherder.mozilla.org/#/jobs?repo=try&revision=cf518f845ccd9d97c2d24b1fa035ca93b27aed04

(In reply to Tooru Fujisawa [:arai] from comment #5) > The test calls `undoCloseTab()` just after removing tab, and if we don't > wait it operates `undo` on the state before removing the tab. > https://searchfox.org/mozilla-central/rev/ > 769222fadff46164f8cc0dc7a0bae5a60dc2f335/browser/base/content/test/general/ > browser_ctrlTab.js#64-66 > > (so... maybe we can just touch that single case tho...) Yeah, doing it for that particular case makes sense (I suppose). Please don't do it for every removeTab call. Thanks!

As discussed with arai on IRC, we take this simple approach now, then later I hope browser guys will make it a cleaner way.

Comment on attachment 8955038 [details] [diff] [review] Wait for next tick in BrowserTestUtils.tabRemoved. Review of attachment 8955038 [details] [diff] [review]: ----------------------------------------------------------------- apparently this fixes all failure related to closed tabs. r+ for now to land bug 1193394. I'll look into this later.

Comment on attachment 8954938 [details] [diff] [review] Wait for an event tick after removing tab before using closed tabs information. clearing r? for now. in any way we should apply different fix ultimately.

https://hg.mozilla.org/integration/mozilla-inbound/rev/2a6481c5aabbaa7a4c9cf657a20c16c4084a6b2d Bug 1441872 - Wait for next tick in BrowserTestUtils.tabRemoved. r=arai

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #17) > It's entirely possible that C++-to-JS calls via XPConnect don't do a > microtask checkpoint on return even if the JS stack is empty. Arguably > that's buggy and we should fix it... We can't do that. Someone tried that and it caused major issues since random JS implemented XPCOM components started to trigger microtask check points at very unexpected times.