Closed Bug 1224077 Opened 4 years ago Closed 4 years ago

MemoryActor should use "runtime: true" rather than "debuggees: this.dbg" when saving snapshots from browser toolbox

Categories

(DevTools :: Memory, defect)

defect
Not set

Tracking

(firefox44 fixed, firefox45 fixed, b2g-v2.5 fixed)

RESOLVED FIXED
Firefox 45
Tracking Status
firefox44 --- fixed
firefox45 --- fixed
b2g-v2.5 --- fixed

People

(Reporter: fitzgen, Assigned: fitzgen)

Details

Attachments

(1 file)

Even though we ahve all globals as debuggees, it affects the root set of the heap snapshot as well as the edges that get traversed.
Comment on attachment 8687277 [details] [diff] [review]
Scope the boundaries of full runtime heap snapshots taken from the MemoryActor properly

Review of attachment 8687277 [details] [diff] [review]:
-----------------------------------------------------------------

::: devtools/server/performance/memory.js
@@ +142,5 @@
>     *
>     * @returns {String} The snapshot id.
>     */
>    saveHeapSnapshot: expectState("attached", function () {
> +    const opts = this.parent instanceof ChromeActor || this.parent instanceof ChildProcessActor

Comments here on what this is doing, and when this would be called (browser toolbox for ChromeActor, and FxOS for the ChildProcessActors?) What is parent an instance of when using this.dbg?
Attachment #8687277 - Flags: review?(jsantell) → review+
ni myself to request uplift once this sits in m-c for a couple days
Flags: needinfo?(nfitzgerald)
https://hg.mozilla.org/mozilla-central/rev/08d771037660
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 45
Comment on attachment 8687277 [details] [diff] [review]
Scope the boundaries of full runtime heap snapshots taken from the MemoryActor properly

Approval Request Comment
[Feature/regressing bug #]: bug 1200446
[User impact if declined]: Saving a heap snapshot for the whole browser rather than a specific tab will be subtly wrong
[Describe test coverage new/current, TreeHerder]: Pretty extensive test suite, been green in m-c for a few days
[Risks and why]: Minimal
[String/UUID change made/needed]: None
Flags: needinfo?(nfitzgerald)
Attachment #8687277 - Flags: approval-mozilla-aurora?
Comment on attachment 8687277 [details] [diff] [review]
Scope the boundaries of full runtime heap snapshots taken from the MemoryActor properly

If this is not a recent regression (i.e. since 42 released) and given the size of the patch (possible risk associated(, I would like this to ride the trains. 

Are our end-users noticing the subtle difference in the memory snapshots and complaining about it? I am just trying to minimize code churn and only uplift patches that are low risk+high end-user impact. Please let me know if you have any concerns.
Attachment #8687277 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
(In reply to Ritu Kothari (:ritu) from comment #8)
> Comment on attachment 8687277 [details] [diff] [review]
> Scope the boundaries of full runtime heap snapshots taken from the
> MemoryActor properly
> 
> If this is not a recent regression (i.e. since 42 released) and given the
> size of the patch (possible risk associated(, I would like this to ride the
> trains. 
> 
> Are our end-users noticing the subtle difference in the memory snapshots and
> complaining about it? I am just trying to minimize code churn and only
> uplift patches that are low risk+high end-user impact. Please let me know if
> you have any concerns.

We didn't expose this functionality to users until 44. No one has noticed, but I'd like to reconsider as fxos branches off of 44, and people debugging fxos are among those who will be most affected. The risk of taking this patch is *very* low.
Flags: needinfo?(rkothari)
(In reply to Nick Fitzgerald [:fitzgen][:nf] from comment #9)
> (In reply to Ritu Kothari (:ritu) from comment #8)
> > Comment on attachment 8687277 [details] [diff] [review]
> > Scope the boundaries of full runtime heap snapshots taken from the
> > MemoryActor properly
> > 
> > If this is not a recent regression (i.e. since 42 released) and given the
> > size of the patch (possible risk associated(, I would like this to ride the
> > trains. 
> > 
> > Are our end-users noticing the subtle difference in the memory snapshots and
> > complaining about it? I am just trying to minimize code churn and only
> > uplift patches that are low risk+high end-user impact. Please let me know if
> > you have any concerns.
> 
> We didn't expose this functionality to users until 44. No one has noticed,
> but I'd like to reconsider as fxos branches off of 44, and people debugging
> fxos are among those who will be most affected. The risk of taking this
> patch is *very* low.

Nick, thanks for the additional context. It was unclear to me from the original aurora uplift request that this may affect fxos end-users. Also, upon looking at the patch again, I noticed that most of the change is in the test automation. I guess I will take this patch in Aurora.
Flags: needinfo?(rkothari)
Thanks! I will try and write better requests in the future :)
Comment on attachment 8687277 [details] [diff] [review]
Scope the boundaries of full runtime heap snapshots taken from the MemoryActor properly

See comment 10 for justification. Aurora44+.
Attachment #8687277 - Flags: approval-mozilla-aurora- → approval-mozilla-aurora+
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.