725804 - Don't add active network requests (XHR, WebSocket, EventSource) to CC graph

Status: RESOLVED FIXED

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

Description

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

The objects are usually kept alive when the network connection is still active.

Comment 1

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

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][[e-mail]]]

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]

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][[e-mail]]]

Attachment: WIP for ws

I'm not yet sure how to handle CORS XHR and EventSource

Comment 5

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

Comment on attachment 596872 [details] [diff] [review]
WIP for ws

mKeepingAlive is true when nsWebSocket has addrefed itself.

Comment 6

[Andrew McCreight [:mccr8]]

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][[e-mail]]]

(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][[e-mail]]]

Attachment: +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]]

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]

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]

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) No longer reading bugmail consistently]

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)]

Comment on attachment 597034 [details] [diff] [review]
+XHR and ES

r=jst

Comment 14

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

https://hg.mozilla.org/mozilla-central/rev/78fde7e54d92