Move setTimeout to WebIDL callback functions

RESOLVED FIXED in mozilla23

Status

()

defect
RESOLVED FIXED
7 years ago
2 months ago

People

(Reporter: bzbarsky, Assigned: bzbarsky)

Tracking

(Blocks 1 bug)

unspecified
mozilla23
x86
macOS
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

No description provided.
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/
> 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.
> Yes, because templates.

That was about the .get(), of course.
Whiteboard: [need review]
Attachment #681069 - Flags: superreview?(peterv)
Attachment #681070 - Flags: superreview?(peterv)
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?
Attachment #681070 - Flags: review?(bugs) → review+
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
Attachment #681069 - Flags: review?(bugs) → review+
Attachment #681070 - Attachment is obsolete: true
Attachment #681070 - Flags: superreview?(peterv)
Attachment #681510 - Flags: superreview?(peterv)
> If Callable() unmarks, could we just assert here that !xpc_IsGrayGCThing(o);

Yes.
Attachment #681069 - Attachment is obsolete: true
Attachment #681069 - Flags: superreview?(peterv)
Attachment #681511 - Flags: superreview?(peterv)
Attachment #681511 - Flags: superreview?(peterv) → superreview+
Attachment #681510 - Flags: superreview?(peterv) → superreview+
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla20 → ---
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
Whiteboard: [need bug 765680 fixed]
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
Flags: in-testsuite- → in-testsuite?
Whiteboard: [need bug 765680 fixed]
Target Milestone: --- → mozilla23
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).
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...
Flags: needinfo?(bent.mozilla)
Oh, and whatever is going on it _only_ happens on b2g(!).
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.
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.
Flags: needinfo?(bent.mozilla)
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....
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.
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(!)).
https://hg.mozilla.org/mozilla-central/rev/1889e0d8372f
https://hg.mozilla.org/mozilla-central/rev/d1ebffdd1d65
Status: REOPENED → RESOLVED
Last Resolved: 7 years ago6 years ago
Resolution: --- → FIXED
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.