Closed
Bug 1180536
Opened 9 years ago
Closed 9 years ago
Use mozilla::ScopeExit to clean up Debugger::addDebuggeeGlobal's consistency on failure
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla42
Tracking | Status | |
---|---|---|
firefox42 | --- | fixed |
People
(Reporter: fitzgen, Assigned: fitzgen)
References
Details
Attachments
(1 file)
No description provided.
Assignee | ||
Comment 1•9 years ago
|
||
Attachment #8632584 -
Flags: review?(sphink)
Comment 2•9 years ago
|
||
Comment on attachment 8632584 [details] [diff] [review] Use mozilla::ScopeExit to clean up Debugger::addDebuggeeGlobal's consistency on failure Review of attachment 8632584 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/vm/Debugger.cpp @@ +3358,5 @@ > + } > + auto zoneDebuggersGuard = MakeScopeExit([&] { > + if (addingZoneRelation) > + zoneDebuggers->popBack(); > + }); Darn, you're right, the ScopeExit thing isn't enough by itself to eliminate addingZoneRelation. You could do it with Maybe<ScopeExit>, but that might just be messier. And given the types involved, I'm not even sure how to do it. You'd need to declare the guard in an outer scope, but give the lambda in the inner, bu the lambda is part of the type... yeah, never mind. @@ +3376,5 @@ > + return false; > + auto allocationsTrackingGuard = MakeScopeExit([&] { > + if (trackingAllocationSites) > + Debugger::removeAllocationsTracking(*global); > + }); Ugh. Again with the clunkiness. Darn it. We could give ScopeExit an unrelease() method... :-( @@ +3388,5 @@ > + globalDebuggersGuard.release(); > + debuggeesGuard.release(); > + zoneDebuggersGuard.release(); > + debuggeeZonesGuard.release(); > + allocationsTrackingGuard.release(); With the ScopeExit stuff, I was half tempted to make a ScopeExitGroup that you'd pass into MakeScopeExit. Then you would just have a single exitGroup.release(). Though come to think it, I'm not entirely sure how to do that. Well, without using vtables or something, which would probably prevent this from getting optimized down. Random wacky types are really annoying.
Attachment #8632584 -
Flags: review?(sphink) → review+
Comment 3•9 years ago
|
||
Probably the best "fix" would be to allow creating a ScopeExit that starts out disabled. It already has the boolean. MakeInactiveScopeExit() or something.
Assignee | ||
Comment 4•9 years ago
|
||
Regardless that this is not as clean as one might have hoped, I do think this is more obviously correct and safe about it. Overall positive. Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ea832514f086
Comment 6•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/6a99b67ccf7f
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox42:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
You need to log in
before you can comment on or make changes to this bug.
Description
•