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

RESOLVED FIXED in mozilla12

Status

()

Core
DOM
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: smaug, Assigned: smaug)

Tracking

12 Branch
mozilla12
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

6 years ago
Patch coming soon.
(Assignee)

Comment 1

6 years ago
Created attachment 590292 [details] [diff] [review]
patch

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

Comment 2

6 years ago
https://tbpl.mozilla.org/?tree=Try&rev=9a3a6d7610ea
(Assignee)

Comment 3

6 years ago
Created attachment 590892 [details] [diff] [review]
patch

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

(class nsIXPConnectWrappedJS; should be removed from elm.h)
Attachment #590892 - Flags: review?(continuation)
(Assignee)

Updated

6 years ago
Blocks: 705582
(Assignee)

Comment 4

6 years ago
Er, the comment about elm.h is about another bug.
Depends on: 720536
This seems to require the patch in 720536, so I'll review that first.
Attachment #590292 - Attachment is obsolete: true
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.
Attachment #590892 - Flags: review?(continuation) → review+
Hardware: x86_64 → All
(Assignee)

Comment 7

6 years ago
(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
(Assignee)

Comment 8

6 years ago
Created attachment 591799 [details] [diff] [review]
patch
Please keep the new timeout event in mind when you check this in (bug 525816).
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.
(Assignee)

Comment 11

6 years ago
https://hg.mozilla.org/mozilla-central/rev/38768b0ef1af
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
(Assignee)

Comment 12

6 years ago
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.
Target Milestone: --- → mozilla12
You need to log in before you can comment on or make changes to this bug.