Closed Bug 751398 Opened 9 years ago Closed 9 years ago

Make jsdService participate in cycle collection for all of its hooks

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla15

People

(Reporter: sfink, Assigned: sfink)

References

Details

Attachments

(1 file)

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.
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.
Attachment #620497 - Flags: review?(khuey)
Depends on: 751396
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
...
Attachment #620497 - Flags: review?(khuey) → review+
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) 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? :)
Blocks: 723712
https://hg.mozilla.org/integration/mozilla-inbound/rev/755d652c481f
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla15
https://hg.mozilla.org/mozilla-central/rev/755d652c481f
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Duplicate of this bug: 506071
You need to log in before you can comment on or make changes to this bug.