js::StringifySavedFrameStack doesn't put the string in the cx's compartment like it promises

RESOLVED FIXED in Firefox 39

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: fitzgen, Assigned: fitzgen)

Tracking

unspecified
mozilla39
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(firefox39 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

It's documentation says:

> The stringified stack out parameter is placed in the cx's compartment.

*Sigh*, but that just is not true.

The first thing it does is maybe enter the SavedFrame's compartment and then the string is built in that possibly different compartment.
Created attachment 8579711 [details] [diff] [review]
Ensure that StringifySavedFrameStack puts its stack string in the cx's compartment
Attachment #8579711 - Flags: review?(bzbarsky)
Sorry, I know you aren't doing reviews right now, but you broke this in 04f34e86aee1 and this is a pretty small patch :P
Created attachment 8579712 [details] [diff] [review]
Same patch but without whitespace changes
Comment on attachment 8579711 [details] [diff] [review]
Ensure that StringifySavedFrameStack puts its stack string in the cx's compartment

This works, but couldn't we actually just let the AutoMaybeEnterFrameCompartment go out of scope before calling finishString()?  Seems to me like that would work fine (albeit with a bit more dependency on StringBuffer internals) and avoid the wrap() call.  Maybe I'm just allergic to wrap() calls: they show up in profiles too much.  :(

Either way should document near the opening curly of the scope we're adding what the scope is there for.
Attachment #8579711 - Flags: review?(bzbarsky) → review+
Created attachment 8580147 [details] [diff] [review]
Ensure that StringifySavedFrameStack puts its stack string in the cx's compartment
Attachment #8579711 - Attachment is obsolete: true
Attachment #8579712 - Attachment is obsolete: true
Attachment #8580147 - Flags: review+
(In reply to Nick Fitzgerald [:fitzgen] from comment #6)
> Try push:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=17c29e1ad7d1

Those failures are fixed in bug 1144108.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/f3988f82aefc
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
status-firefox39: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in before you can comment on or make changes to this bug.