569314 - Allow JS callers to spin event loop until a condition is true

Status: RESOLVED WONTFIX

(Core :: XPCOM, defect)

Description

[Justin Dolske [:Dolske]]

Attachment: Patch v.1

In bug 562258, I want to have some JS code spin the event loop until some condition is true. It seems wasteful to have the JS implemention call processNextEvent() directly, because each spin cycle bounces back and forth from JS to C++ code, though xpconnect. [The browser felt slightly sluggish when doing this; but perhaps I was imagining it, I didn't measure it.]

It would be nice for this case to just have the native code spin away until said condition is true.

I feel a little dirty for having written this, but it seems to work nicely.

Comment 1

[Justin Dolske [:Dolske]]

Hmm, Mook suggested having:

interface nsIFooBar {
    attribute boolean stop;
}

with processNextEventUntil() taking a nsIFooBar... You'd have xpconnect overhead on each loop for calling aFoo->GetStop(), but maybe that's not too bad for a cleaner implementation.

The clever bit being that xpconnect's magic still allows JS callers to do:

var state = { stop : false }
t.processNextEventUntil(state);

Comment 2

[Benjamin Smedberg]

Mook's version isn't any more efficient than a processNextEvent loop... there's still an xpconnect bridge on every time around the event loop. I'd prefer not to add this special API, since spinning nested event loops is a really bad practice in general.

Comment 3

[Justin Dolske [:Dolske]]

bs: The only reason I want this API is for bug 562258; since the tab-modal prompts are not opening a window, they'll need to spin the event loop themselves.

I'm not sure of a better alternative, unless you're suggesting that having the JS code just use the existing processNextEvent() should be sufficient [in which case I'll do some measurements to make sure that's really the case.]

Comment 4

[Benjamin Smedberg]

Hrm, you mean that tab-modal prompts are going to spin event loops? That's a little scary, it doesn't sound hard to end up in a situation where you can exhaust stack space with nested prompts/loops.

Although I guess that without suspendable JS, that's about the best we can do. Ugh.

In any case, yes, a .processNextEvent loop should be sufficient unless it measures poorly.

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I agree with bsmedberg, we don't need a "easy to use" API for this. I'd argue we want spinning the event loop to be hard.

As long as it's possible to spin the event loop, even if it's through complex APIs, I think we should WONTFIX this bug.

PS, aren't alert() already spinning the event loop? Possibly resulting in the problems mentioned in comment 4?

Comment 6

[Benjamin Smedberg]

Yes, but the possibility of re-entering is fairly small because it's app modal. When it becomes tab-modal it's much more likely.

Comment 7

[Justin Dolske [:Dolske]]

(In reply to comment #5)
> I agree with bsmedberg, we don't need a "easy to use" API for this. I'd argue
> we want spinning the event loop to be hard.

I wasn't adding this API for ease of use, rather I was adding it because I assumed (and had an anecdotal observation) that looping over .processNextEvent() from JS could lead to performance problems due to jumping back and forth between JS and C++. Seems that's not a foregone conclusion, so I'll measure to see if that's really a concern or not.

(In reply to comment #6)
> Yes, but the possibility of re-entering is fairly small because it's app modal.
> When it becomes tab-modal it's much more likely.

I had been holding off on the tab-modal prompts for exactly this reason, but when talking with jst he pointed out that since the prompts are only window-modal, you can already get into this state today. Not to mention existing problems with the app automatically launching multiple modal prompts (session restore, HTTP auth, master password).

And, really, more likely than exhausting stack space is the confusing UX resulting from clicking one prompt, but not having it do anything due to other event loops being on top of it. Ick.

But I think we're mitigating the problem by striving to change/eliminate modal prompts in FF4, and pwmgr work to fix the multiple HTTP auth / master password prompts... So, I think FF4 + tab-modal prompts shouldn't be any greater of a problem than the current state of affairs.

But I do still feel rightfully dirty for doing this in JS. ;-)

Comment 8

[Justin Dolske [:Dolske]]

So, I made a little benchmark by taking dbaron's code from http://dbaron.org/log/20100309-faster-timeouts, and slightly modifying it to just do the postMessage() part, 1000 times in a loop. This should result in a nearly worst case test, by spinning the event loop at fast as possible with little real work done per spin.

The numbers seemed to have a fair amount of variation between runs, but tended to settle in a specific range... EG, my OS X debug build was 500ms +/- 40ms. I eyeballed the results from running the test in 3 situations:
  1) Baseline (1 tab, no prompts)
  2) JS loop [alert() with bug 562258 using processNextEvent]
  3) Native loop [alert() with bug 562258 using processNextEventUntil]

I didn't see any significant difference between these cases. Case 2 clearly was not _slower_, and if anything seemed to be somewhat faster (?!).

Conclusion: no obvious advantage for this patch in a worst-case scenario, so do not want.

Comment 9

[Justin Dolske [:Dolske]]

Attachment: Benchmark