1553226 - Crash in [@ nsTSubstring<T>::Assign | CrashReporter::TakeMinidumpForChild]

Status: RESOLVED FIXED

(Toolkit :: Crash Reporting, defect)

Description

[Liz Henry (:lizzard) (relman/hg->git project)]

This bug is for crash report bp-71bcfd82-94c8-4fc7-8bd1-bdbb60190521.

This signature first shows up for the initial nightly 69 build, 20190520215528.

Top 10 frames of crashing thread:

0 xul.dll nsTSubstring<char>::Assign xpcom/string/nsTSubstring.cpp:470
1 xul.dll CrashReporter::TakeMinidumpForChild toolkit/crashreporter/nsExceptionHandler.cpp:3274
2 xul.dll void mozilla::plugins::PluginModuleChromeParent::ProcessFirstMinidump dom/plugins/ipc/PluginModuleParent.cpp:1306
3 xul.dll void mozilla::plugins::PluginModuleChromeParent::ActorDestroy dom/plugins/ipc/PluginModuleParent.cpp:1381
4 xul.dll void mozilla::plugins::PPluginModuleParent::DestroySubtree ipc/ipdl/PPluginModuleParent.cpp:1485
5 xul.dll mozilla::plugins::PPluginModuleParent::OnChannelError ipc/ipdl/PPluginModuleParent.cpp:1449
6 xul.dll nsresult mozilla::detail::RunnableMethodImpl<mozilla::dom::Performance*, void  xpcom/threads/nsThreadUtils.h:1174
7 xul.dll nsThread::ProcessNextEvent xpcom/threads/nsThread.cpp:1175
8 xul.dll NS_ProcessNextEvent xpcom/threads/nsThreadUtils.cpp:486
9 xul.dll void mozilla::ipc::MessagePump::Run ipc/glue/MessagePump.cpp:110

Comment 1

[Eric Rahm [:erahm]]

This code was most recently changed in bug 1547698. My guess is that pd->annotations is null, which seems plausible given it's a UniquePtr.

Comment 2

[Gabriele Svelto [:gsvelto] (PTO)]

I spent some time looking at the code and I now know why it's happening (and why it wasn't caught by the tests): plugins! These are plugin crashes where we're injecting the breakpad exception handler in the plugin process. When the process crashes it will generate a minidump but not through the standard path which adds the annotations so indeed pd->annotations is going to be null.

We haven't caught this in testing because AFAIK we have code paths that test plugin hangs but not crashes so if I it doesn't take too long I'll try to add a test covering this.,

Comment 3

[Gabriele Svelto [:gsvelto] (PTO)]

Even better, this is actually Flash-only because it affects only the Flash plugin's own processes. For other plugins we use the regular exception handler for child processes. Yay for Flash!

Comment 4

[Gabriele Svelto [:gsvelto] (PTO)]

Attachment: Bug 1553226 - Don't attempt to read annotations associated with Flash process crashes

Comment 5

[Gabriele Svelto [:gsvelto] (PTO)]

With this being Flash-specific I couldn't add any tests. I'll try and see if I can test it manually by instancing a Flash plugin and crashing it externally. Somehow someway.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/56fea0bcb2c4
Don't attempt to read annotations associated with Flash process crashes r=froydnj

Comment 7

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/56fea0bcb2c4

Comment 8

[Marcia Knous [:marcia]]

[Tracking Requested - why for this release]: This is one of the early top crashes in 68 beta.

Comment 9

[Julien Cristau [:jcristau]]

Please request beta uplift when you get a chance.

Comment 10

[Gabriele Svelto [:gsvelto] (PTO)]

Comment on attachment 9066687 [details]
Bug 1553226 - Don't attempt to read annotations associated with Flash process crashes

Beta/Release Uplift Approval Request

• User impact if declined: Firefox crashes when the Flash plugin crashes or hangs and is killed by the user
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This introduces a single null check for optional data gathered by the crash reporter.
• String changes made/needed: None

Comment 11

[Julien Cristau [:jcristau]]

Comment on attachment 9066687 [details]
Bug 1553226 - Don't attempt to read annotations associated with Flash process crashes

crash reporter fix for 68.0b5

Comment 12

[Oana Pop-Rus]

https://hg.mozilla.org/releases/mozilla-beta/rev/de5bd9e725c4