810644 - Move setTimeout to WebIDL callback functions

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Comment 1

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/try/rev/79300d6112b4

    1.34 +        // XXXbz Callable() already does xpc_UnmarkGrayObject.  Can we
    1.35 +        // just rely on that?
    1.36 +        JSObject* o = f->Callable();
    1.37 +        xpc_UnmarkGrayObject(o);

Can we add a f->UnmarkGrayObject(), maybe?

    1.75      nsCOMPtr<nsISupports> me(static_cast<nsIDOMWindow *>(this));
    1.86 +    ErrorResult ignored;
    1.87 +    callback->Call(me.get(), handler->GetArgs(), ignored);

Do we need the .get()?

   3.204 +    FallibleTArray<JS::Value> args;
   3.205 +    if (!args.SetCapacity(argCount)) {
   3.206 +      // Non need to drop here, since we already have a non-null mFunction

"No need"

   3.221 +    for (uint32_t idx = 0; idx < argCount; ++idx) {
   3.222 +      *args.AppendElement() = argv[idx+2];
   3.223      }

"idx + 2"

https://hg.mozilla.org/try/rev/da58cba545d3

    1.11  #define NS_ISCRIPTCONTEXTPRINCIPAL_IID \
    1.12 -  { 0xd012cdb3, 0x8f1e, 0x4440, \
    1.13 -    { 0x8c, 0xbd, 0x32, 0x7f, 0x98, 0x1d, 0x37, 0xb4 } }
    1.14 +{ 0xa786f089, 0xafda, 0x4442, \
    1.15 + { 0xb9, 0xe3, 0x06, 0xc3, 0xb3, 0xf0, 0xda, 0x22 } }

That's not the iid you want to update.

    2.12 -nsJSContext::CallEventHandler(nsISupports* aTarget, JSObject* aScope,
    2.13 -                              JSObject* aHandler, nsIArray* aargv,
    2.14 -                              nsIVariant** arv)

\o/ \o/ \o/

Comment 2

[Boris Zbarsky [:bzbarsky]]

> Can we add a f->UnmarkGrayObject(), maybe?

Yes.

> Can we add a f->UnmarkGrayObject(), maybe?

Yes, because templates.  C++ can't decide whether an nsCOMPtr is a ParentObject or a void*.  I _was_ planning to add a comment about that.  ;)

> That's not the iid you want to update.

Thanks.  ;)

Of course the peril of reviewing patches on try is that they are ... not necessarily done.  Like the fact that I need to rework stuff to build on Windows, say.

Comment 3

[Boris Zbarsky [:bzbarsky]]

> Yes, because templates.

That was about the .get(), of course.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Switch setTimeout and setInterval to using WebIDL callback functions. s

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Eliminate the now-unused CallEventHandler. s

Comment 6

[Boris Zbarsky [:bzbarsky]]

Try run for all this stuff: https://tbpl.mozilla.org/?tree=Try&rev=230e83afe4af

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 681070 [details] [diff] [review]
part 2.  Eliminate the now-unused CallEventHandler.   s


>+++ b/dom/base/nsIScriptContext.h
>@@ -1,8 +1,9 @@
>+
Extra newline here?

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 681069 [details] [diff] [review]
part 1.  Switch setTimeout and setInterval to using WebIDL callback functions.   s

> nsGlobalWindow::UnmarkGrayTimers()
> {
>   for (nsTimeout* timeout = mTimeouts.getFirst();
>        timeout;
>        timeout = timeout->getNext()) {
>     if (timeout->mScriptHandler) {
>-      JSObject* o = timeout->mScriptHandler->GetScriptObject();
>-      xpc_UnmarkGrayObject(o);
>+      Function* f = timeout->mScriptHandler->GetCallback();
>+      if (f) {
>+        // XXXbz Callable() already does xpc_UnmarkGrayObject.  Can we
>+        // just rely on that?
>+        JSObject* o = f->Callable();
>+        xpc_UnmarkGrayObject(o);
If Callable() unmarks, could we just assert here that !xpc_IsGrayGCThing(o);

>+  virtual Function* GetCallback() {
{ to next line

>+  virtual const nsTArray<JS::Value>& GetArgs() {
ditto

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 2 with review comment addressed

Comment 10

[Boris Zbarsky [:bzbarsky]]

> If Callable() unmarks, could we just assert here that !xpc_IsGrayGCThing(o);

Yes.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 1 updated to review comments

Comment 12

[Boris Zbarsky [:bzbarsky]]

Pushed:

  https://hg.mozilla.org/integration/mozilla-inbound/rev/39b595bc7d78
  https://hg.mozilla.org/integration/mozilla-inbound/rev/c80717e72675

and then an opt-only bustage fix:

  https://hg.mozilla.org/integration/mozilla-inbound/rev/8dd151b3ce40

Comment 13

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/39b595bc7d78
https://hg.mozilla.org/mozilla-central/rev/c80717e72675
https://hg.mozilla.org/mozilla-central/rev/8dd151b3ce40

Comment 14

[Boris Zbarsky [:bzbarsky]]

Backed out to fix bug 827035: https://hg.mozilla.org/integration/mozilla-inbound/rev/9944f44aff85

I guess I try again once we fix bug 765780.

Comment 15

[Ed Morley [:emorley]]

Merge of backout:
https://hg.mozilla.org/mozilla-central/rev/9944f44aff85

Comment 16

[Ed Morley [:emorley]]

The merge of the backout conflicted with the backout that had been landed on mozilla-central. Mercurial tool resolved it, but managed to double up the removed function. I've fixed manually in:
https://hg.mozilla.org/mozilla-central/rev/d47e21ececb8

Comment 17

[Boris Zbarsky [:bzbarsky]]

Relanded now that bug 765680 is fixed:

   https://hg.mozilla.org/integration/mozilla-inbound/rev/65a949c811fb
   https://hg.mozilla.org/integration/mozilla-inbound/rev/0d1f670b9d86

Comment 18

[Boris Zbarsky [:bzbarsky]]

And backed out again:

   https://hg.mozilla.org/integration/mozilla-inbound/rev/8507944b6211
   https://hg.mozilla.org/integration/mozilla-inbound/rev/7e69fd152f94

to deal with orange due to a timeout dom/workers/test/test_closeOnGC.html on b2g only (b2g M8, looks like).

Comment 19

[Boris Zbarsky [:bzbarsky]]

So this indeed seemed to help that test on b2g.

Ben, any idea why this test would be affected by this patch, and only on b2g?  I tried pushing https://hg.mozilla.org/try/rev/65b0f8e558d2 to see what's going on, and I never see the "Worker is closing" message, but I do see lots of "interval is called" messages which CC successfully...  So presumably the worker is in fact not closing for some reason.  Or perhaps not being collected for some reason...

Comment 20

[Boris Zbarsky [:bzbarsky]]

Oh, and whatever is going on it _only_ happens on b2g(!).

Comment 21

[Boris Zbarsky [:bzbarsky]]

OK, a bit more data.  I added logging to the Finalize functions for a Worker and WorkerPrivate, and here's what I see in the log:

02:46:02     INFO -  I/GeckoDump(  793): 2758 INFO TEST-START | /tests/dom/workers/test/test_closeOnGC.html
02:46:02     INFO -  I/GeckoDump(  793): Finalizing worker
02:46:02     INFO -  I/GeckoDump(  793): Finalizing private:
02:46:02     INFO -  I/GeckoDump(  793): clearTimeouts_worker.js
02:46:02     INFO -  I/GeckoDump(  793): Finalizing worker
02:46:02     INFO -  I/GeckoDump(  793): Finalizing private:
02:46:02     INFO -  I/GeckoDump(  793): blob:3bf2b6bd-3d8c-4179-80ce-17833ab49b57
02:46:02     INFO -  I/GeckoDump(  793): 2759 INFO TEST-INFO | /tests/dom/workers/test/test_closeOnGC.html | interval called 1 times
02:46:02     INFO -  I/GeckoDump(  793): 2760 INFO TEST-INFO | /tests/dom/workers/test/test_closeOnGC.html | XHR responseText: ''
02:46:02     INFO -  I/GeckoDump(  793): Finalizing worker
02:46:02     INFO -  I/GeckoDump(  793): Finalizing private:
02:46:02     INFO -  I/GeckoDump(  793): atob_worker.js
02:46:02     INFO -  I/GeckoDump(  793): Finalizing worker
02:46:02     INFO -  I/GeckoDump(  793): Finalizing private:
02:46:02     INFO -  I/GeckoDump(  793): close_worker.js
02:46:02     INFO -  I/GeckoDump(  793): 2761 INFO TEST-INFO | /tests/dom/workers/test/test_closeOnGC.html | CC succeeded

then lots and lots of firing of the interval timer with nothing else getting collected, until the test times out.  Then the test_contentWorker.html and test_dataURLWorker.html and test_eventDispatch.html tests run, and then I get:

02:46:03     INFO -  I/GeckoDump(  793): Finalizing worker
02:46:03     INFO -  I/GeckoDump(  793): Finalizing private:
02:46:03     INFO -  I/GeckoDump(  793): content_worker.js
02:46:03     INFO -  I/GeckoDump(  793): Finalizing worker
02:46:03     INFO -  I/GeckoDump(  793): Finalizing private:
02:46:03     INFO -  I/GeckoDump(  793): DATA:text/plain,onmessage = function(event) {  postMessage(event.data);};
02:46:03     INFO -  I/GeckoDump(  793): Finalizing worker
02:46:03     INFO -  I/GeckoDump(  793): Finalizing private:
02:46:03     INFO -  I/GeckoDump(  793): closeOnGC_worker.js

So it looks like the upshot is that the worker is simply never finalized in this case while the interval timer is live.  But only on b2g.

Comment 22

[Ben Turner (not reading bugmail, use the needinfo flag!)]

We chatted about this on irc, if we can't figure this out with GC logs then disabling the test on b2g only sounds ok as long as we have a followup to turn it back on.

Comment 23

[Boris Zbarsky [:bzbarsky]]

After some great fun, the issue is that the test relies on the stack scanner to not root the worker, but the stack scanner makes no such promises.  The change to the callstack here means we're not using as much callstack as we used to, so we're not longer wiping the worker pointer off the stack, or something.

I'm going to disable this test on b2g and re-push....

Comment 24

[Andrew McCreight [:mccr8]]

Ugh, that's annoying.  It is too bad that logs weren't working well, as those would have shown you that the object was being rooted by the conservative stack scanner immediately.

Comment 25

[Boris Zbarsky [:bzbarsky]]

To be fair, if we logged things clearly enough to make it obvious they're rooted by the stack scanner in an opt build, the test would have passed (because the debug version puts strings on the stack that affect the stack(!)).

Comment 26

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d1ebffdd1d65
https://hg.mozilla.org/integration/mozilla-inbound/rev/8488f69f8f91

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1889e0d8372f
https://hg.mozilla.org/mozilla-central/rev/d1ebffdd1d65