Last Comment Bug 751398 - Make jsdService participate in cycle collection for all of its hooks
: Make jsdService participate in cycle collection for all of its hooks
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla15
Assigned To: Steve Fink [:sfink] [:s:]
:
: Jason Orendorff [:jorendorff]
Mentors:
: 506071 (view as bug list)
Depends on: 751396
Blocks: 723712
  Show dependency treegraph
 
Reported: 2012-05-02 16:01 PDT by Steve Fink [:sfink] [:s:]
Modified: 2012-07-03 22:11 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Make jsdService participate in cycle collection for all of its hooks (4.39 KB, patch)
2012-05-02 16:01 PDT, Steve Fink [:sfink] [:s:]
khuey: review+
Details | Diff | Splinter Review

Description Steve Fink [:sfink] [:s:] 2012-05-02 16:01:30 PDT
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:] 2012-05-02 16:01:39 PDT
Created attachment 620497 [details] [diff] [review]
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 [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-05-04 12:29:24 PDT
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:] 2012-05-07 10:33:45 PDT
(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? :)
Comment 4 Steve Fink [:sfink] [:s:] 2012-05-15 14:05:33 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/755d652c481f
Comment 5 Ed Morley [:emorley] 2012-05-16 03:34:42 PDT
https://hg.mozilla.org/mozilla-central/rev/755d652c481f
Comment 6 Steve Fink [:sfink] [:s:] 2012-07-03 22:11:35 PDT
*** Bug 506071 has been marked as a duplicate of this bug. ***

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