Closed Bug 1223512 Opened 4 years ago Closed 4 years ago

Need to validate that every edge referent is in the heap snapshot

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)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Big woopsie that we aren't already doing this...

When doing this checking, we can take the opportunity to change edge referents from node ids to pointers since the nodes hash set is frozen after being read.
Comment on attachment 8686784 [details] [diff] [review]
Validate that every edge referent is in the heap snapshot

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

This looks fine to me -- it is validating the integrity of the loaded heap graph, that all edges are known, otherwise the snapshot is corrupt. Correct?
Attachment #8686784 - Flags: review?(shu) → review+
(In reply to Shu-yu Guo [:shu] from comment #3)
> This looks fine to me -- it is validating the integrity of the loaded heap
> graph, that all edges are known, otherwise the snapshot is corrupt. Correct?

Correct.
[Tracking Requested - why for this release]:
(In reply to Nick Fitzgerald [:fitzgen][:nf] from comment #7)
> [Tracking Requested - why for this release]:

Will lead to null pointer dereferencing / undefined behavior / crashes when reading corrupted heap snapshots rather than simply rejecting them.
ni myself to remember to request uplift after this sits in m-c for a day or so.
Flags: needinfo?(nfitzgerald)
https://hg.mozilla.org/mozilla-central/rev/275f6ee72e73
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 45
Comment on attachment 8686784 [details] [diff] [review]
Validate that every edge referent is in the heap snapshot

Approval Request Comment
[Feature/regressing bug #]: bug 1024774
[User impact if declined]: Reading a corrupted heap snapshot will crash the browser rather than simply fail and show a warning
[Describe test coverage new/current, TreeHerder]: Fair amount of heap snapshots test, been green on m-c for a few days.
[Risks and why]: Minimal.
[String/UUID change made/needed]: None
Flags: needinfo?(nfitzgerald)
Attachment #8686784 - Flags: approval-mozilla-aurora?
Comment on attachment 8686784 [details] [diff] [review]
Validate that every edge referent is in the heap snapshot

Fixes a potential crash, let's take it in Aurora44.
Attachment #8686784 - 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.