Closed Bug 1332937 Opened 3 years ago Closed 3 years ago

Turn libcubeb's XASSERT into a synonym for MOZ_ASSERT or MOZ_RELEASE_ASSERT

Categories

(Core :: Audio/Video: cubeb, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox53 --- affected
firefox54 --- fixed

People

(Reporter: ehoogeveen, Assigned: ehoogeveen)

Details

Attachments

(3 files, 1 obsolete file)

libcubeb's XASSERT as currently written doesn't trigger the creation of a crash report on my PC, as evidenced by bug 1332905. It also doesn't set the crash reason, so any crash report that *does* get created won't be as useful as it could be.

XASSERT is currently a release assertion - an assertion failure crashes the browser (or the content process) in opt builds as well as debug builds. This suggests turning it into a synonym for MOZ_RELEASE_ASSERT or perhaps MOZ_DIAGNOSTIC_ASSERT (which crashes in opt builds on Nightly and Aurora).
This changes XASSERT to MOZ_DIAGNOSTIC_ASSERT, to crash on Nightly and Aurora but not on Beta or Release. Most assertions are MOZ_ASSERT, which only crashes in debug builds, but XASSERT is currently a release assertion, so this was the middle ground. Let me know what you think XASSERT should be.

Aside from patching the relevant file, this also adds a patch for update.sh to use the next time we pull libcubeb from upstream. I don't know what the policy is for libcubeb, but this is the mechanism we use in other places. In local testing this worked just fine.
Attachment #8829257 - Flags: review?(padenot)
Even with attachment 8829257 [details] [diff] [review] I don't see the moz crash reason getting set in local testing.

Adding LIBRARY_DEFINES['MOZ_HAS_MOZGLUE'] to gkmedias seems to work, but I'm not sure why it's needed: gkmedias becomes part of XUL, so I would expect the LIBRARY_DEFINES['MOZ_HAS_MOZGLUE'] in XUL to trickle down all the way to libcubeb. Evidently it does not! Is this a quirk of gkmedias?
Attachment #8829258 - Flags: review?(mh+mozilla)
Drive-by: As XASSERT was the only caller of cubeb_crash, can the latter be removed, to prevent other potential non-Breakpad crashes in the future?
Comment on attachment 8829258 [details] [diff] [review]
Part 2: Allow libcubeb and other media libs to set the moz crash reason.

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

::: layout/media/moz.build
@@ +12,5 @@
>  # so we need to keep an independent pseudo-library, as well as the OS_LIBS
>  # on Windows for them to propagate there.
>  Library('gkmedias')
>  
> +LIBRARY_DEFINES['MOZ_HAS_MOZGLUE'] = True

LIBRARY_DEFINES only propagate through FINAL_LIBRARY, not USE_LIBS, that's why you end up requiring this.

At this point, considering we're unlikely to split gkmedias again, I'd rather just add a FINAL_LIBRARY = 'xul' here and remove 'gkmedias' from the USE_LIBS in toolkit/library/moz.build.
Attachment #8829258 - Flags: review?(mh+mozilla)
Rank: 20
Priority: -- → P1
We should fix this upstream, and offer pluggable asserts. I'll do that today.

That said, I'm confused by the fact that the crash reporter does not pick up the crash for you, but it works for other people.
(In reply to Paul Adenot (:padenot) from comment #5)
> That said, I'm confused by the fact that the crash reporter does not pick up
> the crash for you, but it works for other people.

Yeah, I'm not sure why that is. It looks like all the reports we got were on Windows 7, so maybe Windows 10 works differently.
Put a patch up upstream so that we can integrate `MOZ_RELEASE_ASSERT` more nicely: https://github.com/kinetiknz/cubeb/pull/222
(In reply to Paul Adenot (:padenot) from comment #5)
> We should fix this upstream, and offer pluggable asserts. I'll do that today.
> 
> That said, I'm confused by the fact that the crash reporter does not pick up
> the crash for you, but it works for other people.

For reference here's the guts of MOZ_RELEASE_ASSERT: https://dxr.mozilla.org/mozilla-central/rev/8ff550409e1d1f8b54f6f7f115545dbef857be0b/mfbt/Assertions.h#211-236

On Windows we primarily use __debugbreak(), aka int 3, which will cause a crash if you don't have a debugger attached. That's followed by a null deref just in case you do have a debugger and you ignored the breakpoint. And finally, "super just in case", there's a TerminateProcess, though I'm convinced that it's unnecessary (bug 1324093).

On Mac/Linux it's a null deref followed by abort(). I don't know those OS well enough to say whether both are really necessary.

I see that cubeb_crash on all OS currently does abort() first, followed by null deref. Maybe that explains the discrepancy? Perhaps the abort() is not "crashy" enough to go into Breakpad on Windows?
(In reply to Paul Adenot (:padenot) from comment #7)
> Put a patch up upstream so that we can integrate `MOZ_RELEASE_ASSERT` more
> nicely: https://github.com/kinetiknz/cubeb/pull/222

Thanks! Looks like the PR was accepted. So what's next - can we cherry-pick this change from upstream? Do you think XASSERT should map to MOZ_RELEASE_ASSERT?
(In reply to Emanuel Hoogeveen [:ehoogeveen] from comment #9)
> (In reply to Paul Adenot (:padenot) from comment #7)
> > Put a patch up upstream so that we can integrate `MOZ_RELEASE_ASSERT` more
> > nicely: https://github.com/kinetiknz/cubeb/pull/222
> 
> Thanks! Looks like the PR was accepted. So what's next - can we cherry-pick
> this change from upstream? Do you think XASSERT should map to
> MOZ_RELEASE_ASSERT?

We control libcubeb (it's a mozilla project that has been created for Firefox, but is separate for convenience). I'm finishing something unrelated up there that I also need, and then we'll uplift the version we use in mozilla-central using `media/libcubeb/update.sh`.

We'll probably end up mapping to MOZ_RELEASE_ASSERT, yes. I can handle that, it's easy enough.
Changed as per the review comments. I confirmed that this still works, looks green on try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=dad3eb40b0830e810afb980671e48754e36f0ae9

(In reply to Mike Hommey [:glandium] from comment #4)
> LIBRARY_DEFINES only propagate through FINAL_LIBRARY, not USE_LIBS, that's
> why you end up requiring this.

Aha, good to know.

> At this point, considering we're unlikely to split gkmedias again, I'd
> rather just add a FINAL_LIBRARY = 'xul' here and remove 'gkmedias' from the
> USE_LIBS in toolkit/library/moz.build.

Works for me! This patch does just that.
Attachment #8829258 - Attachment is obsolete: true
Attachment #8830794 - Flags: review?(mh+mozilla)
(In reply to Paul Adenot (:padenot) from comment #10)
> We'll probably end up mapping to MOZ_RELEASE_ASSERT, yes. I can handle that,
> it's easy enough.

Great! There's no particular rush here, so I can also update part 1 once the changes are merged if you'd prefer. We still need part 2 to get a crash reason in crash stats.
Attachment #8830794 - Flags: review?(mh+mozilla) → review+
Pushed by paul@paul.cx:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bc1b18684ff1
Allow libcubeb and other media libs to set the moz crash reason. r=glandium
Emanuel, I've landed something equivalent to your first patch in bug 1337328, and I've landed your second part just now. Everything should be fine now, thanks for the reports and patches !
Thanks! Good to have this fixed :)
https://hg.mozilla.org/mozilla-central/rev/bc1b18684ff1
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Attachment #8829257 - Flags: review?(padenot)
Comment on attachment 8836021 [details]
Bug 1332937 - Use MOZ_RELEASE_ASSERT behind cubeb's XASSERT so it's enabled in release.

https://reviewboard.mozilla.org/r/111544/#review112850

Looks good
Attachment #8836021 - Flags: review?(achronop) → review+
Attachment #8829257 - Flags: checkin-
Attachment #8830794 - Flags: checkin+
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Pushed by padenot@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c67282da235f
Use MOZ_RELEASE_ASSERT behind cubeb's XASSERT so it's enabled in release. r=achronop
https://hg.mozilla.org/mozilla-central/rev/c67282da235f
Status: REOPENED → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.