719949 - Unmark listeners in XHR, WebSocket and EventSource if the object is black

Status: RESOLVED FIXED

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

Description

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

Patch coming soon.

Comment 1

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

Attachment: patch

This at least compiles :)
The patch relies on changes I've made to EventListenerManager.

Comment 2

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

https://tbpl.mozilla.org/?tree=Try&rev=9a3a6d7610ea

Comment 3

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

Attachment: patch

I'll ask a review from a DOM peer too.

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

Comment 4

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

Er, the comment about elm.h is about another bug.

Comment 5

[Andrew McCreight [:mccr8]]

This seems to require the patch in 720536, so I'll review that first.

Comment 6

[Andrew McCreight [:mccr8]]

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

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

I don't know anything about these classes, but it looks like nsXMLHttpRequest gets mListenerManager and most of its listener event wrappers from nsXHREventTarget, which you didn't add CanSkip stuff to.  Is that class never directly instantiated or is there some other reason for not adding it?

Are the listener wrappers all null if the listener manager is null?  I'm just curious why those unmarks are guarded.

As I commented in the other bug, we should think about whether these CanSkipInCCs should be closer to CanSkip in a follow-up patch, as they only unmarkGray stuff, and don't actually touch the purple buffer or whatnot.

The xpcprivate.h includes need to be changed back to xpcpublic.h in all 3 places.

r=me with the xpcprivate.h includes reverted.

::: content/base/src/nsEventSource.cpp
@@ +111,5 @@
> +    if (tmp->mListenerManager) {
> +      tmp->mListenerManager->UnmarkGrayJSListeners();
> +      NS_UNMARK_LISTENER_WRAPPER(Open)
> +      NS_UNMARK_LISTENER_WRAPPER(Message)
> +      NS_UNMARK_LISTENER_WRAPPER(Error)

Maybe have these in the order Open, Error, Message to match their declarations?  Though that doesn't match their order in Traverse if IIRC, so whichever you prefer.

Comment 7

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

(In reply to Andrew McCreight [:mccr8] from comment #6)
> I don't know anything about these classes, but it looks like
> nsXMLHttpRequest gets mListenerManager and most of its listener event
> wrappers from nsXHREventTarget,
nsXHREventTarget is just a helper, so that also upload object can extend it.
I'll add upload to bbp later.

> Are the listener wrappers all null if the listener manager is null?
Yes, unless CC unlink is broken :)


> As I commented in the other bug, we should think about whether these
> CanSkipInCCs should be closer to CanSkip in a follow-up patch, as they only
> unmarkGray stuff, and don't actually touch the purple buffer or whatnot.
Well, unmarking can happen only in CanSkip

Comment 8

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

Attachment: patch

Comment 9

[Alex Vincent [:WeirdAl]]

Please keep the new timeout event in mind when you check this in (bug 525816).

Comment 10

[Andrew McCreight [:mccr8]]

It has already been landed on m-c.  Hopefully they won't cause too much merge pain.

It should be safe to not apply this to all timeouts, but we can fix that in a followup.

Comment 11

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

https://hg.mozilla.org/mozilla-central/rev/38768b0ef1af

Comment 12

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

Yeah, this is just an optimization. If ontimeout isn't handled in _SKIP_, that doesn't cause any other
problems than possibly slightly higher CC times in some cases.