Everybody who cares about this function calls it from within libxul.

Nobody calls this from JS, and we have better ways to accomplish the same task in C++

We would like to avoid JS callers relying on .{current,main}Thread. Event loop spinning is one of the major uses, if not the only use, of those attributes. This patch introduces nsIThreadManager::spinEventLoopUntil, a direct analogue of mozilla::SpinEventLoopUntil, and possibly a more efficient alternative to the current situation today. As a proof-of-concept, it converts a number of event loop spin loops in JS to use the new primitive. Some loops can't be converted for boring reasons, and some can't be converted because we need to introduce another primitive in later patches.

Comment on attachment 8877258 [details] [diff] [review] part 2 - remove nsIThreadManager::isMainThread Review of attachment 8877258 [details] [diff] [review]: ----------------------------------------------------------------- I don't really have a problem with this, but it will probably break some addons (a search on dxr showed a fair amount of hits). I think you should at least ping someone from the addons team.

Comment on attachment 8877271 [details] [diff] [review] part 3 - add spinEventLoopUntil to nsIThreadManager Review of attachment 8877271 [details] [diff] [review]: ----------------------------------------------------------------- A few small nits / potential reordering issues in the js conversions and some questions on the implementation so f+ for now. ::: addon-sdk/source/test/addons/content-script-messages-latency/httpd.js @@ +5174,5 @@ > srv.registerDirectory("/", lp); > srv.registerContentType("sjs", SJS_TYPE); > srv.start(port); > > + gThreadManager.spinEventLoopUntil(() => { nit: this can just be | => srv.isStopped()| like the others, right? ::: dom/browser-element/BrowserElementChildPreload.js @@ +437,3 @@ > } > > + return win.modalDepth !== origModalDepth || this._shuttingDown; You've changed the order of the tests (effectively going from a while loop to a do-while loop). Also I *think* 'foo !== bar' is not necessarily the same as !(foo == bar)? I admit my JS crazy-equality-fu is not strong. ::: mobile/android/components/NSSDialogService.js @@ +71,5 @@ > }); > > // Spin this thread while we wait for a result > + Services.tm.spinEventLoopUntil(() => { > + return response != null; again, !(foo === bar) is not the same as foo != bar, but maybe that doesn't matter for null. ::: mobile/android/components/geckoview/GeckoViewPrompt.js @@ +459,5 @@ > this.asyncShowPrompt(aMsg, res => result = res); > > // Spin this thread while we wait for a result > + Services.tm.spinEventLoopUntil(() => { > + return result != undefined; same possible triple vs double operator issue ::: services/sync/tps/extensions/mozmill/resource/modules/assertions.js @@ +597,5 @@ > throw TypeError("waitFor() callback has to return a boolean" + > " instead of '" + type + "'"); > + } > + > + return self.result === true || self.timeIsUp; another 'do' => 'do/while' ::: xpcom/threads/nsIThreadManager.idl @@ +97,5 @@ > + /** > + * Enter a nested event loop on the current thread, waiting on, and > + * processing events until condition.isDone() returns true. > + * > + * If condition.isDone() throws, this function will throw as well. I don't think this statement is currently true, but I might be missing something. AFAICT we just eat the failure and stop spinning. @@ +102,5 @@ > + * > + * C++ code should not use this function, instead preferring > + * mozilla::SpinEventLoopUntil. > + */ > + void spinEventLoopUntil(in nsINestedEventLoopCondition condition); Can you add tests for this? At least one that spins happily and one that throws an exception in |condition|. ::: xpcom/threads/nsThreadManager.cpp @@ +335,5 @@ > } > > +NS_IMETHODIMP > +nsThreadManager::SpinEventLoopUntil(nsINestedEventLoopCondition* aCondition) > +{ Do we need to grab a ref to aCondition? @@ +340,5 @@ > + if (!mozilla::SpinEventLoopUntil([&]() -> bool { > + bool isDone = false; > + nsresult rv = aCondition->IsDone(&isDone); > + // JS failure should be unusual, but... > + if (NS_FAILED(rv)) { I think maybe we want to store |rv| outside of the closure and do a |return rv| instead of |return NS_OK|.

Comment on attachment 8877271 [details] [diff] [review] part 3 - add spinEventLoopUntil to nsIThreadManager Review of attachment 8877271 [details] [diff] [review]: ----------------------------------------------------------------- I was going to make some of the comments Eric made in comment 5 (especially it's unclear to me why you sometimes did spinEventLoopUntil(() => boolean) on a single line, and sometimes on several lines when it could fit on a single line), but I'm not going to repeat them :-). ::: dom/indexedDB/test/unit/test_transaction_lifetimes_nested.js @@ +32,4 @@ > > let eventHasRun; > > thread.dispatch(function() { I think this should become tm.dispatchToMainThread and then we can avoid the "thread" variable. ::: testing/xpcshell/head.js @@ +478,5 @@ > listener.open(); > > // spin an event loop until the debugger connects. > + let tm = Components.classes["@mozilla.org/thread-manager;1"] > + .getService(); nit: move the .getService(); on the previous line. I wonder how many of these Cc["@mozilla.org/thread-manager;1"].getService() could just be converted to Services.tm ... but that's likely outside the scope of what you are trying to do here :-) (and could potentially be done with a scripted rewrite). ::: toolkit/crashreporter/test/unit/crasher_subprocess_tail.js @@ +3,5 @@ > // Let the event loop process a bit before crashing. > if (shouldDelay) { > let shouldCrashNow = false; > + let tm = Components.classes["@mozilla.org/thread-manager;1"] > + .getService(); nit: .getService on the previous line

(In reply to Eric Rahm [:erahm] (please no mozreview requests) from comment #5) > ::: addon-sdk/source/test/addons/content-script-messages-latency/httpd.js > @@ +5174,5 @@ > > srv.registerDirectory("/", lp); > > srv.registerContentType("sjs", SJS_TYPE); > > srv.start(port); > > > > + gThreadManager.spinEventLoopUntil(() => { > > nit: this can just be | => srv.isStopped()| like the others, right? It could be. I did all of them multiline, and only switched enough over that the linter would stop yelling at me. :) I can change them all, I guess. > ::: dom/browser-element/BrowserElementChildPreload.js > @@ +437,3 @@ > > } > > > > + return win.modalDepth !== origModalDepth || this._shuttingDown; > > You've changed the order of the tests (effectively going from a while loop > to a do-while loop). Also I *think* 'foo !== bar' is not necessarily the > same as !(foo == bar)? I admit my JS crazy-equality-fu is not strong. Ah, good catch on the equality bits. Why do you think it's going from a while loop to a do-while loop? mozilla::SpinEventLoopUntil is basically: while (!condition()) { /* spin */ } and that's what we're doing here, no? Is the concern that we'd potentially evaluate the inner window check on the very first iteration and we might break without ever processing events? We can already do that in the original code, though I'm not even sure that could ever happen. > ::: xpcom/threads/nsIThreadManager.idl > @@ +97,5 @@ > > + /** > > + * Enter a nested event loop on the current thread, waiting on, and > > + * processing events until condition.isDone() returns true. > > + * > > + * If condition.isDone() throws, this function will throw as well. > > I don't think this statement is currently true, but I might be missing > something. AFAICT we just eat the failure and stop spinning. We stop spinning, but the error makes mozilla::SpinEventLoopUntil return false, which makes us return NS_ERROR_FAILURE. > @@ +102,5 @@ > > + * > > + * C++ code should not use this function, instead preferring > > + * mozilla::SpinEventLoopUntil. > > + */ > > + void spinEventLoopUntil(in nsINestedEventLoopCondition condition); > > Can you add tests for this? At least one that spins happily and one that > throws an exception in |condition|. I can try. > ::: xpcom/threads/nsThreadManager.cpp > @@ +335,5 @@ > > } > > > > +NS_IMETHODIMP > > +nsThreadManager::SpinEventLoopUntil(nsINestedEventLoopCondition* aCondition) > > +{ > > Do we need to grab a ref to aCondition? Probably can't hurt.

(In reply to Florian Quèze [:florian] [:flo] from comment #6) > ::: dom/indexedDB/test/unit/test_transaction_lifetimes_nested.js > @@ +32,4 @@ > > > > let eventHasRun; > > > > thread.dispatch(function() { > > I think this should become tm.dispatchToMainThread and then we can avoid the > "thread" variable. Sure. I have a followup patch to do that. > ::: testing/xpcshell/head.js > @@ +478,5 @@ > > listener.open(); > > > > // spin an event loop until the debugger connects. > > + let tm = Components.classes["@mozilla.org/thread-manager;1"] > > + .getService(); > > nit: move the .getService(); on the previous line. > > I wonder how many of these Cc["@mozilla.org/thread-manager;1"].getService() > could just be converted to Services.tm ... but that's likely outside the > scope of what you are trying to do here :-) (and could potentially be done > with a scripted rewrite). I respectfully request to not have to do this in this bug. :)

Updated with review comments addressed and some tests.

We did an automated conversion for many of these in another bug, but these instances were either missed or have been added since then.

A number of places in JS need to drain the current thread's event queue, which cannot be done with nsIThreadManager::spinEventLoopUntil, since we need to not wait for an incoming event when attempting to process one. Could attempt to test this one, but would have to think kind of hard about how to do it.

Comment on attachment 8877724 [details] [diff] [review] part 3 - add spinEventLoopUntil to nsIThreadManager Review of attachment 8877724 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/browser-element/BrowserElementChildPreload.js @@ +437,3 @@ > } > > + return win.modalDepth !== origModalDepth || this._shuttingDown; It was == so should be != not !== ::: mobile/android/components/NSSDialogService.js @@ +70,5 @@ > response = data; > }); > > // Spin this thread while we wait for a result > + Services.tm.spinEventLoopUntil(() => response != null); Here you are replacing === with != (should be !==). This will only make a difference if the value of 'response' is ever set to undefined. ::: xpcom/threads/nsIThreadManager.idl @@ +102,5 @@ > + * > + * C++ code should not use this function, instead preferring > + * mozilla::SpinEventLoopUntil. > + */ > + void spinEventLoopUntil(in nsINestedEventLoopCondition condition); The comment here should clarify that the condition will be checked before spinning the loop, as the name of the method indeed sounds like a do {} while() loop here.

Comment on attachment 8877725 [details] [diff] [review] part 4 - use nsIThreadManager::dispatchToMainThread more from JS Review of attachment 8877725 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/extensions/test/browser/head.js @@ +97,5 @@ > > async function promiseAnimationFrame(win = window) { > await new Promise(resolve => win.requestAnimationFrame(resolve)); > > + let {tm} = Services; nit: Just inline Services.tm.dispatch... I think the variable was used only to avoid a long line, but now that we don't need "mainThread.DISPATCH_NORMAL" the line length is reasonable. ::: toolkit/crashreporter/test/unit/crasher_subprocess_tail.js @@ +4,5 @@ > if (shouldDelay) { > let shouldCrashNow = false; > let tm = Components.classes["@mozilla.org/thread-manager;1"] > + .getService(Components.interfaces.nsIThreadManager); > + trailing whitespace

Comment on attachment 8877726 [details] [diff] [review] part 5 - add nsIThreadManager::spinEventLoopUntilEmpty Review of attachment 8877726 [details] [diff] [review]: ----------------------------------------------------------------- Much nicer :-).

Comment on attachment 8877724 [details] [diff] [review] part 3 - add spinEventLoopUntil to nsIThreadManager Review of attachment 8877724 [details] [diff] [review]: ----------------------------------------------------------------- Sorry I mean to r+ this last week!

Comment on attachment 8877726 [details] [diff] [review] part 5 - add nsIThreadManager::spinEventLoopUntilEmpty Review of attachment 8877726 [details] [diff] [review]: ----------------------------------------------------------------- Nice cleanup, just some minor comments. ::: xpcom/threads/nsThreadManager.cpp @@ +362,5 @@ > > +NS_IMETHODIMP > +nsThreadManager::SpinEventLoopUntilEmpty() > +{ > + nsIThread* thread = NS_GetCurrentThread(); Maybe MOZ_ASSERT this? @@ +365,5 @@ > +{ > + nsIThread* thread = NS_GetCurrentThread(); > + > + while (NS_HasPendingEvents(thread)) { > + (void)NS_ProcessNextEvent(thread, false); nit: can you add a /* aParam = */ false style comment here? Also should we bail out if NS_ProcessNextEvent fails rather than continuing to loop? Or is it guaranteed that NS_HasPendingEvents(thread) will return false on the next iteration?

Pushed by nfroyd@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/96a9eabd6a75 part 1 - remove non-MOZILLA_INTERNAL_API NS_IsMainThread(); r=erahm https://hg.mozilla.org/integration/mozilla-inbound/rev/e2d2b68377bf part 2 - remove nsIThreadManager::isMainThread; r=erahm https://hg.mozilla.org/integration/mozilla-inbound/rev/a00c6d0328e6 part 3 - add spinEventLoopUntil to nsIThreadManager; r=erahm,florian https://hg.mozilla.org/integration/mozilla-inbound/rev/0f360c703e46 part 4 - use nsIThreadManager::dispatchToMainThread more from JS; r=florian https://hg.mozilla.org/integration/mozilla-inbound/rev/84e1caaf1aa4 part 5 - add nsIThreadManager::spinEventLoopUntilEmpty; r=erahm,florian

(In reply to Eric Rahm [:erahm] (please no mozreview requests) from comment #16) > ::: xpcom/threads/nsThreadManager.cpp > @@ +362,5 @@ > > > > +NS_IMETHODIMP > > +nsThreadManager::SpinEventLoopUntilEmpty() > > +{ > > + nsIThread* thread = NS_GetCurrentThread(); > > Maybe MOZ_ASSERT this? Probably a good idea. > @@ +365,5 @@ > > +{ > > + nsIThread* thread = NS_GetCurrentThread(); > > + > > + while (NS_HasPendingEvents(thread)) { > > + (void)NS_ProcessNextEvent(thread, false); > > nit: can you add a /* aParam = */ false style comment here? Sure. > Also should we bail out if NS_ProcessNextEvent fails rather than continuing > to loop? Or is it guaranteed that NS_HasPendingEvents(thread) will return > false on the next iteration? It's not guaranteed, because somebody can dispatch an event between NS_ProcessNextEvent finishing and NS_HasPendingEvents performing its check. I think continuing to spin is the safest strategy and the most consistent with existing behavior, both in JS and probably in C++ (where I need to do an audit to check if anything could use something like this).

(In reply to Nathan Froyd [:froydnj] from comment #18) > It's not guaranteed, because somebody can dispatch an event between > NS_ProcessNextEvent finishing and NS_HasPendingEvents performing its check. > I think continuing to spin is the safest strategy and the most consistent > with existing behavior, both in JS and probably in C++ (where I need to do > an audit to check if anything could use something like this). My main concern is an infinite loop where NS_ProcessNextEvent fails, we don't actually process the event and then NS_HasPendingEvents continues to return true. It wasn't clear from a cursory look at the code if this could happen though. Previously on the JS side NS_ProcessNextEvent failing would have caused an exception right?

https://hg.mozilla.org/mozilla-central/rev/96a9eabd6a75 https://hg.mozilla.org/mozilla-central/rev/e2d2b68377bf https://hg.mozilla.org/mozilla-central/rev/a00c6d0328e6 https://hg.mozilla.org/mozilla-central/rev/0f360c703e46 https://hg.mozilla.org/mozilla-central/rev/84e1caaf1aa4