Closed Bug 1416686 Opened 7 years ago Closed 7 years ago

Reduce the uses of IPC_FAIL_NO_REASON in GMPChild.cpp

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 1416686 - Reduce the uses of IPC_FAIL_NO_REASON in GMPChild.cpp.
59 bytes, text/x-review-board-request
away
: review+
cpearce
: review+
Details
Attachment #8927764 - Flags: review?(rweiss)
Attachment #8927764 - Flags: review?(cpearce)
Rank: 15
Priority: -- → P2
The background on this bug is that it helps to diagnose crashes such as:
https://crash-stats.mozilla.com/report/index/bd43c6f5-da41-4cce-99e1-4653c0171111
As being tracked in bug 1416646.

The function GMPChild::GetUTF8LibPath() is returning failure. We'd like to know why it's failing.
Comment on attachment 8927764 [details]
Bug 1416686 - Reduce the uses of IPC_FAIL_NO_REASON in GMPChild.cpp.

https://reviewboard.mozilla.org/r/199048/#review204664

I don't think this will have the effect of collecting the path to the plugin binary in crash reports, which is I think what you were trying to achieve. But it's not actually clear from the bug report or from your commit message what you were trying to achieve.

I also think you didn't give enough context in the bug report and in the patch for Rebecca to understand why you're asking her for review. I assume the reason you're asking her for review is you're looking for approval to collect the path to the plugin binary in crash reports.

::: dom/media/gmp/GMPChild.cpp:552
(Diff revision 3)
>  
>    nsCString libPath;
>    if (!GetUTF8LibPath(libPath)) {
> -    return IPC_FAIL_NO_REASON(this);
> +    return IPC_FAIL(
> +      this,
> +      nsPrintfCString("GetUTF8LibPath returned false with plugin path(%s).",

Note an nsPrintfCString is defined to inherit from nsAutoCStringN<16>:

https://searchfox.org/mozilla-central/rev/a662f122c37704456457a526af90db4e3c0fd10e/xpcom/string/nsPrintfCString.h#23

So that means its internal buffer is 16 characters long. Chances are the path to the plugin is longer than 16 characters.

So this means it's likely that the nsAutoCString will need to allocate a bigger buffer on the heap to fit the printed path string. 

Our crash reports don't collect heap memory, just stack memory. So the path string is likely to be on the heap, and so not be collected in crash reports.

So assuming that your goal here is to collect the path to the binary in crash reports, I don't think you'll achieve that.

If you want to collect the path to the plugin binary in crash reports, you need to ensure it's either on the stack when we abort so its collected in crash reports, or you need to use CrashReporter::AnnotateCrashReport() or similar to ensure the value makes it into crash reports. Probably AnnotateCrashReport is your best bet.
Attachment #8927764 - Flags: review?(cpearce) → review-
(In reply to Chris Pearce (:cpearce) from comment #5)
> Comment on attachment 8927764 [details]
> Bug 1416686 - Reduce the uses of IPC_FAIL_NO_REASON in GMPChild.cpp.
> 
> https://reviewboard.mozilla.org/r/199048/#review204664
> 
> I don't think this will have the effect of collecting the path to the plugin
> binary in crash reports, which is I think what you were trying to achieve.
> But it's not actually clear from the bug report or from your commit message
> what you were trying to achieve.
> 
> I also think you didn't give enough context in the bug report and in the
> patch for Rebecca to understand why you're asking her for review. I assume
> the reason you're asking her for review is you're looking for approval to
> collect the path to the plugin binary in crash reports.
> 
> ::: dom/media/gmp/GMPChild.cpp:552
> (Diff revision 3)
> >  
> >    nsCString libPath;
> >    if (!GetUTF8LibPath(libPath)) {
> > -    return IPC_FAIL_NO_REASON(this);
> > +    return IPC_FAIL(
> > +      this,
> > +      nsPrintfCString("GetUTF8LibPath returned false with plugin path(%s).",
> 
> Note an nsPrintfCString is defined to inherit from nsAutoCStringN<16>:
> 
> https://searchfox.org/mozilla-central/rev/
> a662f122c37704456457a526af90db4e3c0fd10e/xpcom/string/nsPrintfCString.h#23
> 
> So that means its internal buffer is 16 characters long. Chances are the
> path to the plugin is longer than 16 characters.
> 
> So this means it's likely that the nsAutoCString will need to allocate a
> bigger buffer on the heap to fit the printed path string. 
> 
> Our crash reports don't collect heap memory, just stack memory. So the path
> string is likely to be on the heap, and so not be collected in crash reports.
> 
> So assuming that your goal here is to collect the path to the binary in
> crash reports, I don't think you'll achieve that.
> 
> If you want to collect the path to the plugin binary in crash reports, you
> need to ensure it's either on the stack when we abort so its collected in
> crash reports, or you need to use CrashReporter::AnnotateCrashReport() or
> similar to ensure the value makes it into crash reports. Probably
> AnnotateCrashReport is your best bet.

Thank you so much, I didn't know that the crash report can only record the non-heap memory!

But in this path, I composed a string and pass to 

https://searchfox.org/mozilla-central/rev/c633ffa4c4611f202ca11270dcddb7b29edddff8/ipc/glue/ProtocolUtils.cpp#64

which will store in heap.

The point is that it will then invoke ```GMPChild::ProcessingError(Result aCode, const char* aReason)```.

So I copy the aReason into a stack memory in Bug 1416667 and request review again.

I want to make "MOZ_CRASH Reason" more descriptive like
https://crash-stats.mozilla.com/report/index/180124cd-6de2-4214-93c1-d40f50171115

The string of the reason will be like
```aborting because of MsgProcessingError, reason(PGMPChild::AnswerStartPlugin GetUTF8LibPath returned false with plugin path(/home/james/Projects/gecko-mozilla2/obj-x86_64-pc-linux-gnu/dist/bin/gmp-clearkey/0.1).)```

Therefore, I don't modify the patch but request review again.

Thank you.
Hi Rebecca,

As comment 5 said,

Sorry, I didn't provide enough context.

As cpearce said, I'm looking for approval to collect the path to the plugin binary in crash reports.

Currently, I cannot reproduce those crash and there's no clue in crash report.

If we can collect the path which might be composed of some non-latin characters, probably we can do some local test to find our code defects why we made the crash.

Thanks.
Flags: needinfo?(rweiss)
Comment on attachment 8927764 [details]
Bug 1416686 - Reduce the uses of IPC_FAIL_NO_REASON in GMPChild.cpp.

https://reviewboard.mozilla.org/r/199048/#review206544

As per dmajor's comment https://bugzilla.mozilla.org/show_bug.cgi?id=1416667#c7 please use CrashReporter::AnnotateCrashReport for this.
Attachment #8927764 - Flags: review?(cpearce) → review-
I used CrashReporter::AnnotateCrashReport to record the path.

But I have a question need to ask dmajor.

What is the relationship between the code ```CrashReporter::AnnotateCrashReport(NS_LITERAL_CSTRING("IPCShutdownState")```

https://searchfox.org/mozilla-central/rev/919dce54f43356c22d6ff6b81c07ef412b1bf933/dom/ipc/ContentChild.cpp#2968

and the filter tag ```ipc shutdown state``` in Socorro like the column in

https://crash-stats.mozilla.com/signature/?product=Firefox&signature=mozilla%3A%3Agmp%3A%3AGMPChild%3A%3AProcessingError&date=%3E%3D2017-11-14T20%3A57%3A10.000Z&date=%3C2017-11-21T20%3A57%3A10.000Z&_columns=date&_columns=product&_columns=version&_columns=build_id&_columns=platform&_columns=reason&_columns=address&_columns=install_time&_columns=ipc_shutdown_state&_sort=-date&page=1#reports

If I use ```CrashReporter::AnnotateCrashReport(NS_LITERAL_CSTRING("GMPLibraryPath"),```

How will it look like on Socorro? Who will split it into "GMP Library Path"?

Thanks.
Flags: needinfo?(rweiss) → needinfo?(dmajor)
Attachment #8927764 - Flags: review?(rweiss)
Attachment #8927764 - Flags: review?(dmajor)
Attachment #8927764 - Flags: review?(cpearce)
Comment on attachment 8927764 [details]
Bug 1416686 - Reduce the uses of IPC_FAIL_NO_REASON in GMPChild.cpp.

Mozreview bug...

Use bugzilla to r? cpearce again.
Attachment #8927764 - Flags: review?(cpearce)
(In reply to James Cheng[:JamesCheng] from comment #11)
> If I use
> ```CrashReporter::AnnotateCrashReport(NS_LITERAL_CSTRING("GMPLibraryPath"),
> ```
> 
> How will it look like on Socorro? Who will split it into "GMP Library Path"?

After landing, you can file a bug for :willkg to add this field to https://github.com/mozilla-services/socorro/blob/a04d88bc54953bad3ce515024db5eebd642ce4c0/socorro/external/es/super_search_fields.py#L2499 -- this is also the same place where you can make the data public (permissions_needed = []).

(I think the Socorro UI displays the underscores as spaces)
Flags: needinfo?(dmajor)
(In reply to Chris Pearce (:cpearce) from comment #4)
> The background on this bug is that it helps to diagnose crashes such as:
> https://crash-stats.mozilla.com/report/index/bd43c6f5-da41-4cce-99e1-
> 4653c0171111
> As being tracked in bug 1416646.
> 
> The function GMPChild::GetUTF8LibPath() is returning failure. We'd like to
> know why it's failing.

In addition to the existing diagnostics, might I suggest stashing the GetLastError() (on Windows builds) near the point of failure, and later including that in the reporting?
(In reply to David Major [:dmajor] from comment #15)
> (In reply to Chris Pearce (:cpearce) from comment #4)
> > The background on this bug is that it helps to diagnose crashes such as:
> > https://crash-stats.mozilla.com/report/index/bd43c6f5-da41-4cce-99e1-
> > 4653c0171111
> > As being tracked in bug 1416646.
> > 
> > The function GMPChild::GetUTF8LibPath() is returning failure. We'd like to
> > know why it's failing.
> 
> In addition to the existing diagnostics, might I suggest stashing the
> GetLastError() (on Windows builds) near the point of failure, and later
> including that in the reporting?

Good idea! Thanks, I've updated the patch.

BTW, if I don't file a bug for :willkg, will the "GMPLibraryPath" appear on Socorro UI?
(In reply to James Cheng[:JamesCheng] from comment #17)
> BTW, if I don't file a bug for :willkg, will the "GMPLibraryPath" appear on
> Socorro UI?

If you don't take any action, new annotations will be visible in the "Metadata" tab of individual crash reports (for users with crash access), but not in the search UI.
Comment on attachment 8927764 [details]
Bug 1416686 - Reduce the uses of IPC_FAIL_NO_REASON in GMPChild.cpp.

https://reviewboard.mozilla.org/r/199048/#review207414

You are in a race with bug 1402519. If that bug lands first, you can remove ifdef MOZ_CRASHREPORTER. If you land first, please advise gsvelto to update his patch.
Attachment #8927764 - Flags: review?(dmajor) → review+
(In reply to David Major [:dmajor] from comment #21)
> Comment on attachment 8927764 [details]
> Bug 1416686 - Reduce the uses of IPC_FAIL_NO_REASON in GMPChild.cpp.
> 
> https://reviewboard.mozilla.org/r/199048/#review207414
> 
> You are in a race with bug 1402519. If that bug lands first, you can remove
> ifdef MOZ_CRASHREPORTER. If you land first, please advise gsvelto to update
> his patch.

Thank you for reminding!

I think I will wait for that bug being landed.
Comment on attachment 8927764 [details]
Bug 1416686 - Reduce the uses of IPC_FAIL_NO_REASON in GMPChild.cpp.

https://reviewboard.mozilla.org/r/199048/#review208310
Attachment #8927764 - Flags: review?(cpearce) → review+
Blocks: 1420797
Pushed by jacheng@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ba381cfa946f
Reduce the uses of IPC_FAIL_NO_REASON in GMPChild.cpp. r=cpearce,dmajor
https://hg.mozilla.org/mozilla-central/rev/ba381cfa946f
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: