Closed Bug 976120 Opened 11 years ago Closed 11 years ago

No stack trace for MOZ_ASSERT on debug emulator builds

Categories

(Firefox OS Graveyard :: General, defect)

Product:
Firefox OS Graveyard
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: jgriffin, Assigned: ahal)

References

Details

Attachments

(2 files, 1 obsolete file)

Patch 1.0 - emulator.py should set crash env, check for crashes anytime a MarionetteException occurs
12.49 KB, patch
jgriffin
: review+
Details | Diff | Splinter Review
Patch 2.0 - make b2g crashreporting respect crash environment
1.20 KB, patch
fabrice
: review+
Details | Diff | Splinter Review
We're not getting stack traces for MOZ_ASSERTs on debug emulator builds. See e.g., https://tbpl.mozilla.org/php/getParsedLog.php?id=34891270&tree=Cedar&full=1#error25
Ted, any idea what we're missing here?
Flags: needinfo?(ted)
I think this is a dupe of bug 929128. I think the in-process unwinding stuff that XPCOM_DEBUG_BREAK=stack uses is not implemented for gonk.
Flags: needinfo?(ted)
But MOZ_ASSERT doesn't use that does it?
Oh, you're right. I think the key bit here is that that assertion seems to be happening in the child process. I think someone filed a bug on this recently--our test harnesses aren't doing the right thing in the face of a child process crash. We should just quit the test run when this happens and then dump the stack from the child process minidump.
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #4) > Oh, you're right. I think the key bit here is that that assertion seems to > be happening in the child process. I think someone filed a bug on this > recently--our test harnesses aren't doing the right thing in the face of a > child process crash. We should just quit the test run when this happens and > then dump the stack from the child process minidump. Any ideas how we can do this? In this case, the child process crash isn't even causing a test to fail.
Back in the days of testing e10s Fennec, we added a MOZ_CRASHREPORTER_SHUTDOWN environment variable. When set, the code that handled content process crashes would immediately exit the app upon noting a child process crash: http://mxr.mozilla.org/mozilla-esr17/source/mobile/xul/chrome/content/browser.js#2529 B2G has a similar handler, so we could do something similar: http://hg.mozilla.org/mozilla-central/annotate/4b6103d24d1e/b2g/chrome/content/shell.js#l1199
I'll take a look at this. It also looks like we don't honour MOZ_CRASHREPORTER_NO_REPORT for child crashes, we use the app.reportCrashes pref instead.
Okay, so that would be why we didn't get a stack trace at the end of the run. You'll want to support both so that we immediately quit and get a stack.
We'll also need to re-write the marionette runner a bit. Right now it isn't setting either environment variable for emulator tests and afaik, the only way to get environment passed into the b2g process is to kill and restart it. This probably involves refactoring b2ginstance.py to use mozrunner.
That sounds like a PITA. Maybe we can just add some privileged APIs to flip these settings on instead?
Yeah, I've been trying to get this working without much success. Even worse is the fact that even if I did get it working every test harness using marionette would also need to be updated so we don't end up restarting the b2g process twice. So I'm definitely on board with a hacky workaround. I'll try just calling nsIEnvironment at some point during the setup.
I was thinking more along the lines of just adding some chrome API that we could call. We already have nsICrashReporter, maybe just stick some extra fields on that? If that's not your bag I could whip up a patch for it, then we'd just have to make the B2G chrome respect it.
Sure, if you think that's worth it. From the marionette runner's perspective it's either call this new api or call the nsIEnvironment api, I don't really care which. If you think creating a new api would benefit other areas too, then let's do that. In that case I don't mind looking into it, but it would probably take me significantly longer..
This was all that's needed to get it working locally. I'm inclined to just land this over doing anything fancier, but if you want to whip up a quick patch to add some additional api's to nsICrashReporter, I can hold off and update this patch when you do so.
Assignee: nobody → ahalberstadt
Status: NEW → ASSIGNED
Flags: needinfo?(ted)
Note the above patch still doesn't address the child process crash problem the bug was originally filed for. That's coming soon.
Works for me!
Flags: needinfo?(ted)
I liked the idea of putting a check_for_crash call in a decorator, so took it one step further. This patch will call check_for_crash anytime any exception deriving from MarionetteException is thrown, regardless of where the call to marionette server is coming from.
Attachment #8382523 - Attachment is obsolete: true
Attachment #8383054 - Flags: review?(jgriffin)
There's another problem even after these two patches. If there is a crash and the application shuts down, the emulator will keep running until a mozharness/buildbot timeout occurs. We need some mechanism for polling the b2g process or something. I'll look into this issue in a separate bug though. In the meantime, these two patches are safe to land.
Blocks: 977635
Comment on attachment 8383054 [details] [diff] [review] Patch 1.0 - emulator.py should set crash env, check for crashes anytime a MarionetteException occurs Review of attachment 8383054 [details] [diff] [review]: ----------------------------------------------------------------- Clever!
Attachment #8383054 - Flags: review?(jgriffin) → review+
Comment on attachment 8383073 [details] [diff] [review] Patch 2.0 - make b2g crashreporting respect crash environment Review of attachment 8383073 [details] [diff] [review]: ----------------------------------------------------------------- I'm not a peer of this code but this looks sane.
Attachment #8383073 - Flags: review?(ted) → review+
Comment on attachment 8383073 [details] [diff] [review] Patch 2.0 - make b2g crashreporting respect crash environment Review of attachment 8383073 [details] [diff] [review]: ----------------------------------------------------------------- Thanks, I'll get peer review to be thorough. Blame shows that fabrice has done the majority of reviews here.
Attachment #8383073 - Flags: review+ → review?(fabrice)
Comment on attachment 8383073 [details] [diff] [review] Patch 2.0 - make b2g crashreporting respect crash environment Review of attachment 8383073 [details] [diff] [review]: ----------------------------------------------------------------- ::: b2g/chrome/content/shell.js @@ +127,5 @@ > this.CrashSubmit.pruneSavedDumps(); > > + // check for environment affecting crash reporting > + let env = Cc["@mozilla.org/process/environment;1"] > + .getService(Ci.nsIEnvironment); nit: align .getService() with ["@mozilla..."] @@ +131,5 @@ > + .getService(Ci.nsIEnvironment); > + let shutdown = env.get("MOZ_CRASHREPORTER_SHUTDOWN"); > + if (shutdown) { > + let appStartup = Cc["@mozilla.org/toolkit/app-startup;1"] > + .getService(Ci.nsIAppStartup); here too
Attachment #8383073 - Flags: review?(fabrice) → review+
(In reply to Andrew Halberstadt [:ahal] from comment #19) > There's another problem even after these two patches. If there is a crash > and the application shuts down, the emulator will keep running until a > mozharness/buildbot timeout occurs. We need some mechanism for polling the > b2g process or something. I'll look into this issue in a separate bug though. I filed bug 969146 on this general problem.
Bug 977635 is actually a separate issue and only affects jobs that don't use mozrunner. Bug 969146 affects jobs that *do* use mozrunner. The former is probably low priority and I have a fix for the latter.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: