Closed Bug 614578 Opened 9 years ago Closed 9 years ago

Remove nsAutoGCRoot

Categories

(Core :: JavaScript Engine, defect)

x86
All
defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
status2.0 --- wanted

People

(Reporter: igor, Assigned: igor)

References

(Blocks 1 open bug)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 file)

The conservative GC should take about all users of nsAutoGCRoot. But we should not just remove the class. Rather its usage should be replaced by an  auto class that would assert in the constructor and destructor that code executes under the JS request.
> The conservative GC should take about all users of nsAutoGCRoot.

That's not necessarily true...  Consider this code pattern:

  JSObject *obj = getMyObject()
  MyClass* priv = obj->getPrivate();
  // work with priv

Since |obj| is unused after the getPrivate() call, the compiler can reuse its stack slot... but it might be the only thing keeping |priv| alive, no?
(In reply to comment #1)
> Since |obj| is unused after the getPrivate() call, the compiler can reuse its
> stack slot... but it might be the only thing keeping |priv| alive, no?

Very good point. Indeed this is similar to the string/chars issue. I guess we need some form of an autoroot class.
Attached patch v1Splinter Review
I monitored the code and it turned out that that the pattern from the comment 1 do not exits among nsAutoRoot users. So this patch is based on Gal's patch from the bug comment 5 613319 with extra changes to ensure that the code always runs a JS request when there is a root on the stack.

With the patch the only users of AddRoot functionality is JS_GetPropertyDescArray and extensions/jssh/nsJSSh.cpp where the of AddRoot in the latter can lead to uncollectable loop. But the case of JS_GetPropertyDescArray is a hard one and clearly shows that some form of AddRoot is necessary if we want to avoid the burden of rooting with JSPropertyDescArray callers. But these worries are for the bug 613319.
Assignee: general → igor
Attachment #493439 - Flags: review?(mrbkap)
We don't have to block on this, but we should definitely take this if its done in time.
blocking2.0: final+ → ---
status2.0: --- → wanted
Blocks: 616927
No longer blocks: compartmentGC
Attachment #493439 - Flags: review?(mrbkap) → review+
http://hg.mozilla.org/tracemonkey/rev/fc0384edf67b
Whiteboard: fixed-in-tracemonkey
http://hg.mozilla.org/mozilla-central/rev/fc0384edf67b
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Depends on: 595208
You need to log in before you can comment on or make changes to this bug.