Closed Bug 1416667 Opened 8 years ago Closed 8 years ago

Use MOZ_CRASH_UNSAFE_PRINTF in GMPChild::ProcessingError to take aReason into account.

Categories

(Core :: Audio/Video: GMP, enhancement, P2)

Product:
Core
Component:
Audio/Video: GMP
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
firefox59 --- fixed

People

(Reporter: JamesCheng, Assigned: JamesCheng)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Bug 1416667 - Use MOZ_CRASH_UNSAFE_PRINTF in GMPChild::ProcessingError
59 bytes, text/x-review-board-request
cpearce
: review+
Details
We didn't use the aReason in GMPChild::ProcessingError(Result aCode, const char* aReason) [1]. We could use MOZ_CRASH_UNSAFE_PRINTF to make it more meaningful in Crash Report [1] https://searchfox.org/mozilla-central/rev/fe75164c55321e011d9d13b6d05c1e00d0a640be/dom/media/gmp/GMPChild.cpp#627
Attachment #8927748 - Flags: review?(cpearce)
very simple modification, I'll let cpearce to review it directly.
Comment on attachment 8927748 [details] Bug 1416667 - Use MOZ_CRASH_UNSAFE_PRINTF in GMPChild::ProcessingError https://reviewboard.mozilla.org/r/199016/#review204052 ::: dom/media/gmp/GMPChild.cpp:637 (Diff revision 1) > + > switch (aCode) { > case MsgDropped: > _exit(0); // Don't trigger a crash report. > case MsgNotKnown: > - MOZ_CRASH("aborting because of MsgNotKnown"); > + MOZ_CRASH_UNSAFE_PRINTF("aborting because of MsgNotKnown, reason(%s)", aReason); It looks to me like aReason will in most cases be the name of the function that failed. We can already get this by looking at the call stack of the crash report. So I don't think this give us any new information. Or did I miss something?
Attachment #8927748 - Flags: review?(cpearce) → review-
Depends on: 1416686
Rank: 15
Priority: -- → P2
I've explained in Bug 1416686 comment 7. I found that the value of MAX_PATH in Windows is 260 PATH_MAX on Mac is 1024 and on Linix is 4096. So I declare the buffer size with 5120. Thanks.
David: Can you offer us your advice on this please? James is trying to collect the path to the CDM binary in crash reports order to debug an error we're tracking. We're failing to load the CDM in some cases, and we abort when we do and so get a crash report. I suspect we're failing to load the CDM because there's something funny about the path; maybe it's some charset that we can't handle. James is trying to add the path to the IPC_FAIL message in bug 1416686, and then trying to store that message's string on the stack when we abort in the patch in this bug so that it's collected in crash reports. Is that the best way to collect the path? Does it need to be volatile in order to avoid potentially being optimized away by the compiler. Or should James be adding the path to CrashReporter::AnnotateCrashReport() or something like that?
Flags: needinfo?(dmajor)
(In reply to Chris Pearce (:cpearce) from comment #6) > James is trying to add the path to the IPC_FAIL message in bug 1416686, and > then trying to store that message's string on the stack when we abort in the > patch in this bug so that it's collected in crash reports. For bug 1416686, CrashReporter::AnnotateCrashReport is the way to go. Note that those annotations are private by default, unless you ask the Socorro people to make them public. Private is good in this case, as you definitely don't want paths that potentially contain usernames etc. going into the public MozCrashReason field. For this bug, note that the comment above MOZ_CRASH_UNSAFE_PRINTF says that it requires data review. Also, MOZ_CRASH_UNSAFE_PRINTF was really only added to grandfather some old callers and it would be great not to increase its numbers. And you're going to get capped at 1024 chars anyway: https://dxr.mozilla.org/mozilla-central/rev/b056526be38e96b3e381b7e90cd8254ad1d96d9d/mfbt/Assertions.h#298 So I think it would be better to use AnnotateCrashReport here too.
Flags: needinfo?(dmajor)
Thanks David. James, please use CrashReporter::AnnotateCrashReport. Thanks.
Attachment #8927748 - Flags: review?(cpearce) → review-
As comment 7 said, MOZ_CRASH_UNSAFE_PRINTF will copy the message into the buffer which is 1024 chars(also not a heap memory but a bss section). https://searchfox.org/mozilla-central/rev/919dce54f43356c22d6ff6b81c07ef412b1bf933/mfbt/Assertions.cpp#54 So I just pass the aReason to MOZ_CRASH_UNSAFE_PRINTF.
Comment on attachment 8927748 [details] Bug 1416667 - Use MOZ_CRASH_UNSAFE_PRINTF in GMPChild::ProcessingError https://reviewboard.mozilla.org/r/199016/#review206594 Please use AnnotateCcr
Attachment #8927748 - Flags: review?(cpearce) → review+
Pushed by jacheng@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/22453f19f050 Use MOZ_CRASH_UNSAFE_PRINTF in GMPChild::ProcessingError r=cpearce
(In reply to Chris Pearce (:cpearce) from comment #11) > Comment on attachment 8927748 [details] > Bug 1416667 - Use MOZ_CRASH_UNSAFE_PRINTF in GMPChild::ProcessingError > > https://reviewboard.mozilla.org/r/199016/#review206594 > > Please use AnnotateCcr What happened here?
Flags: needinfo?(jacheng)
I guess it's a typo(AnnotateCrashReport) or something that cpearce reminds me of using AnnotateCrashReport in Bug 1416686. ni cpearce to make sure if I misunderstand anything.
Flags: needinfo?(jacheng) → needinfo?(cpearce)
https://hg.mozilla.org/mozilla-central/rev/22453f19f050
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Yeah, oops, we should be using CrashReporter::AnnotateCrashReport.
Flags: needinfo?(cpearce)
Pushed by cpearce@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/2e9497af3729 Backed out changeset 22453f19f050. r=backout
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: