720536 - unmark ELM listeners

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

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

Attachment: patch

(class nsIXPConnectWrappedJS; should be removed from elm.h)

I could just call ->GetJSObject() since that does unmarking, 
but I want to be explicit in these cases.

Comment 1

[Andrew McCreight [:mccr8]]

Comment on attachment 590896 [details] [diff] [review]
patch

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

A concern here is that it seems like overkill to drag in all of XPConnect, and xpcprivate, for what seems like a single thing you need, namely marking a held object black.  I think you don't even need to do that.  The idea is just to mark whatever object is held by the wrapped JS black, right?

I think this is sufficient to do this:
      JSObject *o;
      wjs->GetJSObject(&o);
You don't even need to explicitly unmarkGray the JSObject because for CC safety that is done any time you hand out an object.

Those two lines would replace all of this:
      nsXPCWrappedJS* w = static_cast<nsXPCWrappedJS*>(wjs.get());
      if (w) {
        JSObject* o = w->GetJSObjectPreserveColor();
        if (o) {
          xpc_UnmarkGrayObject(o);
        }
      }

Then I think you can use xpcpublic.h instead of xpcprivate.h.  I don't know how that affects what you need to put in the Makefile, if at all.  It looks to me like that can be done in both places.

Does that make sense to you?

I'll try to understand the actual listener stuff now. :)

Comment 2

[Andrew McCreight [:mccr8]]

You may need to do an xpc_UnmarkGrayObject after all, because XPCJSObjectHolder::GetJSObject doesn't ungray, but in that case your old code would have just crashed I think, so maybe it isn't relevant.

Comment 3

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

(In reply to Andrew McCreight [:mccr8] from comment #1)
> A concern here is that it seems like overkill to drag in all of XPConnect,
> and xpcprivate
I'm going to drag xpcprivate to everywhere :)
Well, in fact it is already used in many place where I need it.

(In reply to Andrew McCreight [:mccr8] from comment #2)
> You may need to do an xpc_UnmarkGrayObject after all, because
> XPCJSObjectHolder::GetJSObject doesn't ungray, but in that case your old
> code would have just crashed I think, so maybe it isn't relevant.
What would have crashed and where?

And this is about nsXPCWrappedJS, not XPCJSObjectHolder.
nsXPCWrappedJS::GetJSObject does clear the gray bit.
I just don't think calling GetJSObject should be the way to unmark gray object,
when unmarking is the only thing to do to an object.
It is a side effect of GetJSObject that is unmarks.
(and I can mumble about virtual call)

Comment 4

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

Also, I need the xpc_UnmarkGrayObject for nsJSEventListener stuff anyway

Comment 5

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

Comment on attachment 590896 [details] [diff] [review]
patch

Asking re-review.

Btw, nsDOMEventTargetHelper stuff will go away reasonable soon.

Comment 7

[Andrew McCreight [:mccr8]]

(In reply to Olli Pettay [:smaug] from comment #3)
> (In reply to Andrew McCreight [:mccr8] from comment #1)
> > A concern here is that it seems like overkill to drag in all of XPConnect,
> > and xpcprivate
> I'm going to drag xpcprivate to everywhere :)

Okay, but what exactly do you need from there?

> What would have crashed and where?

Never mind, you are QIing to the interface version of XPCWrappedJS.
 
> And this is about nsXPCWrappedJS, not XPCJSObjectHolder.
> nsXPCWrappedJS::GetJSObject does clear the gray bit.
> I just don't think calling GetJSObject should be the way to unmark gray
> object,
> when unmarking is the only thing to do to an object.
> It is a side effect of GetJSObject that is unmarks.
> (and I can mumble about virtual call)

Well, you can use GetJSObject, then call xpc_UnmarkGrayObject on it, if you prefer, or add an assertion that the object isn't gray after you get it.  The actual overhead of the getting part is not very high.  Or if you are really concerned about the virtual call overhead, you could add a method like UnmarkGrayJSObject to nsIXPConnectWrappedJS and then call that.

(In reply to Olli Pettay [:smaug] from comment #4)
> Also, I need the xpc_UnmarkGrayObject for nsJSEventListener stuff anyway

That's in xpcpublic.h, not xpcprivate.h.

Comment 8

[Andrew McCreight [:mccr8]]

Also, xpc_UnmarkGrayObject does a NULL check, so you don't need to guard calls to it with your own NULL checks.

Comment 9

[Andrew McCreight [:mccr8]]

(In reply to Andrew McCreight [:mccr8] from comment #7)
> Or if you are
> really concerned about the virtual call overhead, you could add a method
> like UnmarkGrayJSObject to nsIXPConnectWrappedJS and then call that.

Though I guess all this IDL stuff gets turned into virtual calls so never mind, that probably won't help.
 
> That's in xpcpublic.h, not xpcprivate.h.

To follow up, when I made the change I suggested above to this patch, it compiled with xpcpublic.  I didn't test it otherwise so maybe it doesn't work. ;)

Comment 10

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

Oh, ok, it is in public after all. I is the nsXPCWrappedJS which is in private.
If you really want me to use only xpcpublic, I can, but I don't quite see the reason.

Null check can be removed though.

Comment 11

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

(In reply to Olli Pettay [:smaug] from comment #10)
> I can, but I don't quite see
> the reason.
> 
...since if we can call non-virtual method easily, we should, IMHO.

Comment 12

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

Attachment: without null checks

Comment 13

[Andrew McCreight [:mccr8]]

It just seems like a lot of stuff to drag in just for this one thing.  I guess you could add a function that wraps that call to put in xpcpublic, so you would have only direct calls.  But you'd have an additional indirection.

Maybe I'm being too picky. I guess I'm just not comfortable with making the judgement about whether it is appropriate to include xpcprivate myself.  If an XPConnect peer says it is okay to include xpcprivate in these files (and whatever Makefile stuff is needed for that, as seen in this patch) then I'm okay with it.  Here are the files where it is added (from this patch and some others up for review now):
  content/base/src/nsEventSource.cpp
  content/base/src/nsWebSocket.cpp
  content/base/src/nsXMLHttpRequest.cpp
  content/events/src/nsEventListenerManager.cpp
  content/base/src/nsFrameMessageManager.cpp

jst, do you have any thoughts on this?

Comment 14

[Andrew McCreight [:mccr8]]

Err, CC didn't go through, so I'll have to add jst again.

Comment 15

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

Boo, XPConnect peers don't want xpcprivate to be used outside XPConnect.
I guess this takes couple of hours to duplicate some code to xpcpublic

Comment 16

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

Attachment: using the xpc stuff

Comment 17

[Andrew McCreight [:mccr8]]

Comment on attachment 591108 [details] [diff] [review]
using the xpc stuff

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

Just so I understand what is going on here, the point of mWrappedJS is to avoid having to do that QI every time you UnmarkGrayJSListeners unless you need to?

A listener is never somehow replaced so that you might have to update the mWrappedJS flag later?  I looked over the ELM file and didn't see anything like that.

Looks good.

Comment 18

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

(In reply to Andrew McCreight [:mccr8] from comment #17)
> Just so I understand what is going on here, the point of mWrappedJS is to
> avoid having to do that QI every time you UnmarkGrayJSListeners unless you
> need to?
Yup. QIing is virtual-heavy stuff. Better to avoid it, especially since we have extra bits
in the listener struct.

> A listener is never somehow replaced so that you might have to update the
> mWrappedJS flag later? 
No. Event listeners can be added or removed, not updated.

Comment 19

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

https://hg.mozilla.org/mozilla-central/rev/4fc8228cbfa5