Expose Enter/ExitProfileLabel to JS

RESOLVED WONTFIX

Status

()

RESOLVED WONTFIX
6 years ago
5 years ago

People

(Reporter: BenWa, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

6 years ago
Created attachment 628117 [details] [diff] [review]
patch

Exposing EnterProfileLabel will let scripts add their own pseudo stackwalking label.

I think this will break down if we're using JS Generators since we wont be pushing/poping in stack properly.
Attachment #628117 - Flags: review?(ehsan)

Comment 1

6 years ago
Comment on attachment 628117 [details] [diff] [review]
patch

Please do proper null checking in EnterProfileLabel.

Also, the last hunk doesn't seem relevant, so please remove it.

r=me with the above.
Attachment #628117 - Flags: review?(ehsan) → review+
(Reporter)

Comment 2

6 years ago
(In reply to Ehsan Akhgari [:ehsan] from comment #1)
> Comment on attachment 628117 [details] [diff] [review]
> patch
> 
> Please do proper null checking in EnterProfileLabel.
> 
> Also, the last hunk doesn't seem relevant, so please remove it.
> 
> r=me with the above.

It is relevant.
(Reporter)

Comment 3

6 years ago
Comment on attachment 628117 [details] [diff] [review]
patch

re-requesting review. See previous comment.
Attachment #628117 - Flags: review+ → review?(ehsan)

Comment 4

6 years ago
Comment on attachment 628117 [details] [diff] [review]
patch

Hmm, I'm not seeing mozilla_sampler_call_exit_no_handle used anywhere in your patch, but if you want it in, r=me (with the nit about null checking).
Attachment #628117 - Flags: review?(ehsan) → review+
(Reporter)

Comment 5

6 years ago
It's called by 'ExitProfileLabel()'.

Comment 6

6 years ago
(In reply to Benoit Girard (:BenWa) from comment #5)
> It's called by 'ExitProfileLabel()'.

You're right, I'm blind!
(Reporter)

Comment 8

6 years ago
Added a trivial null check but it had a typo. I decided to backout and run the patch through try:
https://hg.mozilla.org/integration/mozilla-inbound/rev/466d337a1996
If backing out, please can you use a message of the form:
"{Backout|Backing out|Backed out} <changeset> (<bug_number>) reason"

Just because when marking merges (either manually as done at present; or when the script does it using the same logic), it's really easy to miss messages like:
"Bug 759532 - [Backout] Expose Enter/ExitProfileLabel to JS. r=ehsan"

...and I may end up (or the script will definitely end up) marking bugs closed when they were actually backed out already before the inbound -> m-c merge.

The script that mak created for backouts (that also makes multiple backouts a breeze) uses this format, plus fills out the bug number for you:
https://wiki.mozilla.org/User:Mak77

Thanks! :-)
Assignee: nobody → bgirard
What's happening with this?
(Reporter)

Comment 11

6 years ago
It needs more testing to make sure it doesn't conflict with the JS profiling feature. I don't have time to work on this at the moment. I can explain what needs to be done if we have any takers.
Assignee: bgirard → nobody
(Reporter)

Comment 12

5 years ago
WONTFIX since we support dynamic labels and markers now.
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.