move event loop spinning from JS into C++

RESOLVED FIXED in Firefox 56

Status

()

Core
XPCOM
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: froydnj, Assigned: froydnj)

Tracking

unspecified
mozilla56
Points:
---

Firefox Tracking Flags

(firefox56 fixed)

Details

Attachments

(5 attachments, 1 obsolete attachment)

Comment hidden (empty)
(Assignee)

Comment 1

a year ago
Created attachment 8877257 [details] [diff] [review]
part 1 - remove non-MOZILLA_INTERNAL_API NS_IsMainThread()

Everybody who cares about this function calls it from within libxul.
Attachment #8877257 - Flags: review?(erahm)
(Assignee)

Comment 2

a year ago
Created attachment 8877258 [details] [diff] [review]
part 2 - remove nsIThreadManager::isMainThread

Nobody calls this from JS, and we have better ways to accomplish the
same task in C++
Attachment #8877258 - Flags: review?(erahm)
(Assignee)

Comment 3

a year ago
Created attachment 8877271 [details] [diff] [review]
part 3 - add spinEventLoopUntil to nsIThreadManager

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.
Attachment #8877271 - Flags: review?(florian)
Attachment #8877271 - Flags: review?(erahm)
(Assignee)

Updated

a year ago
Blocks: 1350432
Attachment #8877257 - Flags: review?(erahm) → review+
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.
Attachment #8877258 - Flags: review?(erahm) → review+
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|.
Attachment #8877271 - Flags: review?(erahm) → feedback+
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
Attachment #8877271 - Flags: review?(florian) → feedback+
(Assignee)

Comment 7

a year ago
(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.
(Assignee)

Comment 8

a year ago
(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. :)
(Assignee)

Comment 9

a year ago
Created attachment 8877724 [details] [diff] [review]
part 3 - add spinEventLoopUntil to nsIThreadManager

Updated with review comments addressed and some tests.
Attachment #8877724 - Flags: review?(florian)
Attachment #8877724 - Flags: review?(erahm)
(Assignee)

Updated

a year ago
Attachment #8877271 - Attachment is obsolete: true
(Assignee)

Comment 10

a year ago
Created attachment 8877725 [details] [diff] [review]
part 4 - use nsIThreadManager::dispatchToMainThread more from JS

We did an automated conversion for many of these in another bug, but
these instances were either missed or have been added since then.
Attachment #8877725 - Flags: review?(florian)
(Assignee)

Comment 11

a year ago
Created attachment 8877726 [details] [diff] [review]
part 5 - add nsIThreadManager::spinEventLoopUntilEmpty

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.
Attachment #8877726 - Flags: review?(florian)
Attachment #8877726 - Flags: review?(erahm)
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.
Attachment #8877724 - Flags: review?(florian) → review+
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
Attachment #8877725 - Flags: review?(florian) → review+
Comment on attachment 8877726 [details] [diff] [review]
part 5 - add nsIThreadManager::spinEventLoopUntilEmpty

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

Much nicer :-).
Attachment #8877726 - Flags: review?(florian) → review+
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!
Attachment #8877724 - Flags: review?(erahm) → review+
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?
Attachment #8877726 - Flags: review?(erahm) → review+

Comment 17

a year ago
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
(Assignee)

Comment 18

a year ago
(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?
You need to log in before you can comment on or make changes to this bug.