Make jsdService participate in cycle collection for all of its hooks

RESOLVED FIXED in mozilla15

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: sfink, Assigned: sfink)

Tracking

unspecified
mozilla15
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
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.
Attachment #620497 - Flags: review?(khuey)
(Assignee)

Updated

5 years ago
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+
(Assignee)

Comment 3

5 years ago
(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? :)
(Assignee)

Updated

5 years ago
Blocks: 723712
(Assignee)

Comment 4

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/755d652c481f
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla15

Comment 5

5 years ago
https://hg.mozilla.org/mozilla-central/rev/755d652c481f
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Assignee)

Updated

5 years ago
Duplicate of this bug: 506071
You need to log in before you can comment on or make changes to this bug.