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)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox42 --- fixed

People

(Reporter: fitzgen, Assigned: fitzgen)

References

Details

Attachments

(1 file)

      No description provided.
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+
Probably the best "fix" would be to allow creating a ScopeExit that starts out disabled. It already has the boolean. MakeInactiveScopeExit() or something.
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
https://hg.mozilla.org/mozilla-central/rev/6a99b67ccf7f
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: