Closed
Bug 751398
Opened 12 years ago
Closed 12 years ago
Make jsdService participate in cycle collection for all of its hooks
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla15
People
(Reporter: sfink, Assigned: sfink)
References
Details
Attachments
(1 file)
4.39 KB,
patch
|
khuey
:
review+
|
Details | Diff | Splinter Review |
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•12 years ago
|
||
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)
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•12 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 | ||
Comment 4•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/755d652c481f
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla15
Comment 5•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/755d652c481f
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•