751398 - Make jsdService participate in cycle collection for all of its hooks

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Steve Fink [:sfink] [:s:]]

jsdService holds onto JS objects in its various hook members (eg mScriptHook), and it is ridiculously easy for these to form cycles. A typical scenario is to create a property on the global named 'jsd' that holds onto the service, then set jsd.scriptHook to some functions. Those functions refer to the global.

I suspect Firebug won't be helped by cycle collecting these. I bet it's already working around this by nulling things out. Poor Firebug. But it still makes proper tests way too hard to write.

Comment 1

[Steve Fink [:sfink] [:s:]]

Attachment: Make jsdService participate in cycle collection for all of its hooks

mccr8 sez khuey may be one of the few people around who actually know how to add things to the cycle collector. Please redirect the review if not.

Comment 2

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 620497 [details] [diff] [review]
Make jsdService participate in cycle collection for all of its hooks

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

::: js/jsd/jsd_xpc.cpp
@@ -2449,5 @@
>  
>  /******************************************************************************
>   * debugger service implementation
>   ******************************************************************************/
> -NS_IMPL_THREADSAFE_ISUPPORTS1(jsdService, jsdIDebuggerService)

Is the fact that this is no longer threadsafe going to be a problem?  If somebody tries to AddRef/Release a cycle collected object off the main thread the browser will abort.

@@ +2460,5 @@
> +/* NS_IMPL_CYCLE_COLLECTION_10(jsdService, ...) */
> +
> +NS_IMPL_CYCLE_COLLECTION_CLASS(jsdService)
> +NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(jsdService)
> +NS_IMPL_CYCLE_COLLECTION_UNLINK_NSCOMPTR(mErrorHook)

Normally we indent these so you have

NS...UNLINK_BEGIN
  NS...UNLINK_NSCOMPTR
...

Comment 3

[Steve Fink [:sfink] [:s:]]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #2)
> Comment on attachment 620497 [details] [diff] [review]
> Make jsdService participate in cycle collection for all of its hooks
> 
> Review of attachment 620497 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/jsd/jsd_xpc.cpp
> @@ -2449,5 @@
> >  
> >  /******************************************************************************
> >   * debugger service implementation
> >   ******************************************************************************/
> > -NS_IMPL_THREADSAFE_ISUPPORTS1(jsdService, jsdIDebuggerService)
> 
> Is the fact that this is no longer threadsafe going to be a problem?  If
> somebody tries to AddRef/Release a cycle collected object off the main
> thread the browser will abort.

As I understand it, JSD has only pretended to be threadsafe for a quite a while. There are multiple places where it doesn't handle multiple threads properly, and there are assertions that will barf if you attempt to use it on any thread other than the main thread. People do complain about this, because it means Firebug can't debug workers, but at this point it's a known limitation.

Which doesn't mean that there isn't some code path that grabs a jsdService reference before checking what thread it's on. But that's what we have tests for, right? :)

Comment 4

[Steve Fink [:sfink] [:s:]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/755d652c481f

Comment 5

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/755d652c481f