Last Comment Bug 725804 - Don't add active network requests (XHR, WebSocket, EventSource) to CC graph
: Don't add active network requests (XHR, WebSocket, EventSource) to CC graph
Status: RESOLVED FIXED
[snappy:P2]
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: 12 Branch
: x86_64 Linux
: -- normal (vote)
: ---
Assigned To: Olli Pettay [:smaug]
:
Mentors:
Depends on:
Blocks: 716598
  Show dependency treegraph
 
Reported: 2012-02-09 13:50 PST by Olli Pettay [:smaug]
Modified: 2012-05-30 07:22 PDT (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WIP for ws (1.12 KB, patch)
2012-02-13 17:39 PST, Olli Pettay [:smaug]
continuation: review+
Details | Diff | Review
+XHR and ES (8.88 KB, patch)
2012-02-14 07:55 PST, Olli Pettay [:smaug]
continuation: review+
jduell.mcbugs: review+
jst: review+
Details | Diff | Review

Description Olli Pettay [:smaug] 2012-02-09 13:50:51 PST
The objects are usually kept alive when the network connection is still active.
Comment 1 Olli Pettay [:smaug] 2012-02-12 09:35:00 PST
So, I was looking at XHR code. Am I right that if XHR has mChannel, and mChannel's GetNotificationCallbacks returns XHR itself, mChannel owns XHR.
But is there any better way to check whether necko owns XHR (or EventSource or WebSocket)?
Comment 2 Olli Pettay [:smaug] 2012-02-12 10:02:11 PST
GetNotificationCallbacks approach wouldn't quite work, since it is addreffing operation, and
I need the information at times when addref/release is prohibited.
Comment 3 Jason Duell [:jduell] (needinfo? me) 2012-02-13 13:03:04 PST
We talked about this a bit on IRC.  

For HTTP/FTP (everything other than Websocket channels) we know that necko holds a ref to the listener between a successful AsyncOpen and OnStopRequest.  (Callbacks will be held for life of channel IIRC).   

For websockets Ollie mentioned he might be able to use mKeepingAlive as a proxy for whether we're holding onto CC object, but I'm not sure it's a good metric.  For one, there's a window from nsWebSocket::Init until the first UpdateMustKeepAlive call (which is when mKeepingAlive is first set to true).  Also, when mKeepingAlive is false, that just means the nsWebSocket removes it's own ref to *itself* not to mOwner.  mOwner is held for the duration of a nsWebSocket's lifetime (so maybe it's actually easy: just assume it always holds a ref).   Note that this is not the same as the actual necko channel (WebSocketChannel) which can disappear earler/later than nsWebSocket. 

Hope that helps.
Comment 4 Olli Pettay [:smaug] 2012-02-13 17:39:42 PST
Created attachment 596872 [details] [diff] [review]
WIP for ws

I'm not yet sure how to handle CORS XHR and EventSource
Comment 5 Olli Pettay [:smaug] 2012-02-14 05:04:16 PST
Comment on attachment 596872 [details] [diff] [review]
WIP for ws

mKeepingAlive is true when nsWebSocket has addrefed itself.
Comment 6 Andrew McCreight [:mccr8] 2012-02-14 05:39:59 PST
Comment on attachment 596872 [details] [diff] [review]
WIP for ws

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

I'm assuming that if mKeepingAlive is true, that the nsWebSocket must be alive.

Interesting case.  If I get around to fixing up bug 717500, we could take advantage of that in the GC, too.

::: content/base/src/nsWebSocket.cpp
@@ +438,5 @@
>  
>  NS_IMPL_CYCLE_COLLECTION_CLASS(nsWebSocket)
>  
>  NS_IMPL_CYCLE_COLLECTION_CAN_SKIP_BEGIN(nsWebSocket)
> +  bool isBlack = false;

Just initialize isBlack to tmp->IsBlack(), no need that I can see for assignment in the if.

@@ +447,5 @@
>        NS_UNMARK_LISTENER_WRAPPER(Error)
>        NS_UNMARK_LISTENER_WRAPPER(Message)
>        NS_UNMARK_LISTENER_WRAPPER(Close)
>      }
> +    if (!isBlack) {

You don't unmarkGray if it is black because that implies any JS it points to will already be black?
Comment 7 Olli Pettay [:smaug] 2012-02-14 05:41:35 PST
(In reply to Andrew McCreight [:mccr8] from comment #6)
> You don't unmarkGray if it is black because that implies any JS it points to
> will already be black?
yes
Comment 8 Olli Pettay [:smaug] 2012-02-14 07:55:12 PST
Created attachment 597034 [details] [diff] [review]
+XHR and ES

This relies on OnStopRequest to be called always after AsyncOpen.
Jonas, if I read CORS correctly, it gives the same behavior, right?

I'll add some generic UnmarkWrapperCache() in a followup bug.

https://tbpl.mozilla.org/?tree=Try&rev=a40b199a54b0
Comment 9 Andrew McCreight [:mccr8] 2012-02-15 11:39:03 PST
Comment on attachment 597034 [details] [diff] [review]
+XHR and ES

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

Looks reasonable to me, assuming that the object is known to be alive whenever the flag is true, of course, as I'm not familiar with that.
Comment 10 Jason Duell [:jduell] (needinfo? me) 2012-02-15 14:16:50 PST
Comment on attachment 597034 [details] [diff] [review]
+XHR and ES

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

I don't know CC well enough to have any comfort being a real reviewer here.  I'm fine with the changes if those who do know CC are ok with it, and you verify (via Jonas or someone else) that CORS will also guarantee that OnStop gets called eventually (if it doesn't I assume that's a bug).

> mKeepingAlive is true when nsWebSocket has addrefed itself.

Yes, while mKeepingAlive is true the WS holds a ref to itself, so I guess that's equiv to black IIUC.
Comment 11 Jason Duell [:jduell] (needinfo? me) 2012-02-15 14:17:44 PST
Comment on attachment 596872 [details] [diff] [review]
WIP for ws

Assuming this patch is obsolete since the code got rolled into the other XHR/ES patch.
Comment 12 Jonas Sicking (:sicking) 2012-02-15 20:40:55 PST
I don't really know this part of cycle collection well enough that my review would add a whole lot. But yes, OnStopRequest will always be called, even when CORS listener-proxies are involved.
Comment 13 Johnny Stenback (:jst, jst@mozilla.com) 2012-02-15 21:10:01 PST
Comment on attachment 597034 [details] [diff] [review]
+XHR and ES

r=jst
Comment 14 Olli Pettay [:smaug] 2012-02-16 13:06:33 PST
https://hg.mozilla.org/mozilla-central/rev/78fde7e54d92

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