Last Comment Bug 719949 - Unmark listeners in XHR, WebSocket and EventSource if the object is black
: Unmark listeners in XHR, WebSocket and EventSource if the object is black
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: 12 Branch
: All All
: -- normal (vote)
: mozilla12
Assigned To: Olli Pettay [:smaug]
:
Mentors:
Depends on: 720536
Blocks: 705582
  Show dependency treegraph
 
Reported: 2012-01-20 12:12 PST by Olli Pettay [:smaug]
Modified: 2012-05-30 07:19 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (17.12 KB, patch)
2012-01-20 12:18 PST, Olli Pettay [:smaug]
no flags Details | Diff | Splinter Review
patch (10.45 KB, patch)
2012-01-23 15:23 PST, Olli Pettay [:smaug]
continuation: review+
Details | Diff | Splinter Review
patch (9.55 KB, patch)
2012-01-26 07:46 PST, Olli Pettay [:smaug]
no flags Details | Diff | Splinter Review

Description Olli Pettay [:smaug] 2012-01-20 12:12:38 PST
Patch coming soon.
Comment 1 Olli Pettay [:smaug] 2012-01-20 12:18:49 PST
Created attachment 590292 [details] [diff] [review]
patch

This at least compiles :)
The patch relies on changes I've made to EventListenerManager.
Comment 2 Olli Pettay [:smaug] 2012-01-20 12:28:50 PST
https://tbpl.mozilla.org/?tree=Try&rev=9a3a6d7610ea
Comment 3 Olli Pettay [:smaug] 2012-01-23 15:23:27 PST
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)
Comment 4 Olli Pettay [:smaug] 2012-01-23 15:24:52 PST
Er, the comment about elm.h is about another bug.
Comment 5 Andrew McCreight [:mccr8] 2012-01-23 16:07:03 PST
This seems to require the patch in 720536, so I'll review that first.
Comment 6 Andrew McCreight [:mccr8] 2012-01-25 13:51:08 PST
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] 2012-01-26 07:45:57 PST
(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] 2012-01-26 07:46:26 PST
Created attachment 591799 [details] [diff] [review]
patch
Comment 9 Alex Vincent [:WeirdAl] 2012-01-26 11:25:46 PST
Please keep the new timeout event in mind when you check this in (bug 525816).
Comment 10 Andrew McCreight [:mccr8] 2012-01-26 11:28:34 PST
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] 2012-01-26 12:30:22 PST
https://hg.mozilla.org/mozilla-central/rev/38768b0ef1af
Comment 12 Olli Pettay [:smaug] 2012-01-26 12:38:11 PST
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.

Note You need to log in before you can comment on or make changes to this bug.