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

Status: RESOLVED FIXED

(DevTools :: Memory, defect)

Description

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

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 1

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Validate that every edge referent is in the heap snapshot

Comment 2

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fbfd061d73b9

Comment 3

[Shu-yu Guo [:shu]]

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?

Comment 4

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(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.

Comment 5

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

New try push because task cluster was on the fritz: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f9ab21b33900

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/275f6ee72e73

Comment 7

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

[Tracking Requested - why for this release]:

Comment 8

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(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.

Comment 9

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

ni myself to remember to request uplift after this sits in m-c for a day or so.

Comment 10

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/275f6ee72e73

Comment 11

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

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

Comment 12

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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.

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/0141686cbdc0

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/0141686cbdc0