Closed Bug 1461326 Opened 4 years ago Closed 4 years ago

add release assertions to validate ClientManager/ClientHandle

Categories

(Core :: DOM: Core & HTML, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla62
Tracking Status
firefox61 --- fixed
firefox62 --- fixed

People

(Reporter: bkelly, Assigned: bkelly)

References

Details

Attachments

(1 file, 1 obsolete file)

Over in bug 1461181 we are seeing some unexplained nullptr de-refs.  This bug will add some diagnostic assertions to help track down what is happening.
Unfortunately bug 1461181 does not trigger in nightly or dev-edition.  So we need release assertions here.
Summary: add diagnostic assertions to validate ClientManager/ClientHandle → add release assertions to validate ClientManager/ClientHandle
Andrea, this patch adds some release assertions to dom/clients/manager code.  The goal is to try to isolate the crashes occurring in bug 1461181.  Unfortunately the crashes are only occurring in release/beta, so I can't use diagnostic asserts here.

The various nullptr checks should be somewhat self-explanatory.  I also added some magic-number checks to make sure that TLS isn't returning a garbage pointer to us.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ad6250b1bb064565c7b65245c2322b6d27fd043d
Comment on attachment 8975521 [details] [diff] [review]
Add some release assertions to dom/clients/manager code. r=baku

See comment 2 for the initial description.

I also added some magic values and checking for the static TLS index value.  I'm worried maybe something is stomping on that.
Attachment #8975521 - Flags: review?(amarchesini)
Priority: -- → P3
Attachment #8975521 - Flags: review?(amarchesini) → review+
Pushed by bkelly@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/205e5ff205d3
Add some release assertions to dom/clients/manager code. r=baku
https://hg.mozilla.org/mozilla-central/rev/205e5ff205d3
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Please request approval on this when you get a chance so we can get this into tomorrow's b6 build.
Flags: needinfo?(bkelly)
Comment on attachment 8975521 [details] [diff] [review]
Add some release assertions to dom/clients/manager code. r=baku

Approval Request Comment
[Feature/Bug causing the regression]: Unexplained crashes in bug 1461181.
[User impact if declined]: The crashes in bug 1461181 don't make a lot of sense.  This patch includes additional release assertions to try to narrow down what could be happening.
[Is this code covered by automated tests?]: This code is exercised a lot in automation, but the crashes in question don't trigger there.
[Has the fix been verified in Nightly?]: Its been on nightly for 12 hours or so.
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: Moderate
[Why is the change risky/not risky?]: This patch adds full release assertions and not diagnostic assertions.  This is necessary to help track down the problem on beta.  If there is a widespread problem, though, we run the risk of producing a high beta crash rate.  Of course, we need to know if that problem exists or not, so I think we should attempt this uplift now.
[String changes made/needed]: None
Flags: needinfo?(bkelly)
Attachment #8975521 - Flags: approval-mozilla-beta?
Comment on attachment 8975521 [details] [diff] [review]
Add some release assertions to dom/clients/manager code. r=baku

Adds diagnostic asserts to hopefully help narrow down the cause of bug 1461181. Approved for 61.0b6.
Attachment #8975521 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Blocks: 1465103
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.