Closed Bug 1410528 Opened 2 years ago Closed 2 years ago

Use canned binary to produce stack dumps for JS shell tasks

Categories

(Core :: JavaScript Engine, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: sfink, Assigned: sfink)

References

(Depends on 1 open bug)

Details

Attachments

(5 files, 6 obsolete files)

21.50 KB, patch
ted
: review+
jonco
: review+
Details | Diff | Splinter Review
3.05 KB, patch
jonco
: review+
Details | Diff | Splinter Review
10.50 KB, patch
ted
: review+
Details | Diff | Splinter Review
1.73 KB, patch
glandium
: review+
Details | Diff | Splinter Review
3.65 KB, patch
glandium
: review+
Details | Diff | Splinter Review
I would really like to get back to bug 837969 someday, but I just don't have time, and I was thinking that I don't need to block the functionality on the mechanism.

So this bug is for going to the ancient gecko revision that I successfully produced an LD_PRELOAD breakpad injector from, building it, then using that as a canned binary during shell runs.

I can then return to bug 837969 at a later time and fix it properly, possibly converting it into a toolchain task that produces the binary.

(At this point, in order to finish up bug 837969, I would not only need to go back and dig out of my memory the way that ted recommended doing it, but also handle all the breakpad changes that have accumulated in the meantime and most likely invalidated most of the build system and other work I did there. Such is the penalty for procrastination.)
Attached file libbreakpadinjector.so for linux64 (obsolete) —
This is the binary that works on linux64, at least. sha512 digest is e3f5b6c20c202577163c1adbd15ca73167ed21dede4026d6dd221eb8c5901e9950360fc0203225effbc73541f9891a41cd81ce172f48643a2a8f1889f35db16b and it is uploading to tooltool now.
Did you want to take this?
Flags: needinfo?(sphink)
Priority: -- → P3
Assignee: nobody → sphink
Flags: needinfo?(sphink)
Looks like I'll need minidump_stackwalk and dump_syms binaries too. And it appears that generating breakpad symbols is not straightforward if you're not building the whole browser via mach. :(
Hi Ted! Remember this stuff? (from bug 837969). I never did get back around to redoing it the proper way, but people have been asking more and more, so I thought I'd cheat and build a binary from my old code and just upload it as a tooltool binary. Whaddaya think?

Sample push (but note this has a patch that crashes every 10 invocations of the JS shell, which produces a very large output file. Probably ought to tone that down some.): https://treeherder.mozilla.org/#/jobs?repo=try&revision=c55bf9a8250a3f9bfb64d28d7bf1ca06ae48c0de&selectedJob=139632127

Also note that this is linux64 only for now, and I need to disable it for incompatible builds (eg SM(asan) and SM(pkg), though perhaps I should fix the latter.) But it's working well enough to get feedback now.
Attachment #8922094 - Flags: feedback?(ted)
Comment on attachment 8922094 [details] [diff] [review]
When running via autospider.sh, make the shell generate a minidump on crashes

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

I am generally supportive of this approach. I think you should best-effort document how the shared library was built for now, just for sanity's sake.

Also, you should use the minidump-stackwalk binary from https://dxr.mozilla.org/mozilla-central/source/testing/config/tooltool-manifests/linux64/releng.manifest , because that's what we use elsewhere in automation. It will automatically fetch symbols from symbols.mozilla.org, which is nice because we have some system library symbols there.

I suspect writing a toolchain task to build your shared library against stock Breakpad with source that lives in a hg.mo user repo or a GitHub repo would probably be easier than trying to shoehorn it into our build system, honestly. You could use whatever build system makes your life easiest.

::: js/src/devtools/automation/autospider.py
@@ +369,5 @@
> +        shutil.copy(os.path.join(DIR.tooltool, "breakpad-tools", "dump_syms"),
> +                    os.path.join(hostdir, 'dump_syms'))
> +        run_command([
> +            'make',
> +            'buildsymbols',

If you don't actually need the symbols.zip, you can just run `make recurse-syms` instead, which will put the symbols in dist/crashreporter-symbols but not zip them up:
https://dxr.mozilla.org/mozilla-central/rev/aa958b29c149a67fce772f8473e9586e71fbdb46/Makefile.in#255
https://dxr.mozilla.org/mozilla-central/rev/aa958b29c149a67fce772f8473e9586e71fbdb46/config/recurse.mk#84

@@ +373,5 @@
> +            'buildsymbols',
> +            'MOZ_CRASHREPORTER=1',
> +            # In automation, the compile step generates the .sym files.
> +            'MOZ_AUTOMATION=',
> +            'MOZ_SOURCE_REPO=file://' + DIR.source,

Does this stuff not get set for standalone JS builds? I think it happens as part of configure now, it ought to be fixable. (It's not going to make much of a difference here though, tbh, it's only used to turn source paths into links to hg.mo.)

@@ +379,5 @@
> +        ], check=True)
> +
> +        # Generate stacks when this script exits.
> +        venv_python = os.path.join(OBJDIR, "_virtualenv", "bin", "python")
> +        atexit.register(run_command,

I don't think I've ever seen anything use atexit in Python. Is there a reason you can't just run this at the end of the script? Is the control flow weird? If that's the problem you could do like:

import contextlib

@contextlib.contextmanager
def check_for_crashes():
  # do setup
  yield
  # check for crashes

with check_for_crashes():
  # harness logic goes here
Attachment #8922094 - Flags: feedback?(ted) → feedback+
Ok, I know this is a little crazy. Let me explain what's going on. Note that I'm flagging both ted and jonco for review, because the relevant parts are so intermingled that I couldn't come up with a meaningful separation.

I want to be able to load a shared library that produces minidumps on crashes.

I can't simply set LD_PRELOAD to the library, because we sometimes run 32-bit processes under 64-bit processes, and setting LD_PRELOAD to the wrong arch will spit out a warning, which will break some some test automation because it's parsing the output. So I added a --dll flag to the shell.

But our cross-platform way of loading shared libraries is NSPR, which currently we do not require for the shell build unless you enable ctypes. I don't want to add a new dependency, since most people are using --enable-posix-nspr-emulation (it's the default). And I've only implemented the shared library on Linux (or reimplemented, rather; my ancient version worked for linux, osx, and Windows, but I don't have a good enough development environment for the other two at the moment. And I'd want to do it as a followon anyway.)

So I used #ifdef __linux__ and used native dlopen/dlsym.

Finally, if you just do this naively, you end up with an unknown number of *expected* minidumps, for a crash test and for allow-unhandled-oom tests. I could tweak the test harness to delete the minidumps after the fact for the crash test, but the unhandled oom tests should really only suppress the crash in the case of an unhandled oom, not a crash for another reason. So I added a mechanism to dynamically disable the minidump generation, that works via a "gBreakpadInjectorEnabled" symbol in the breakpad injector lib.

Finally, I didn't want to *also* add a --dll flag to jsapi-tests. So this library is used via LD_PRELOAD for that (since it isn't spawning mismatched binaries or anything). No modifications are necessary for this case, though. (Earlier iterations did.)

Feel free to tell me that some or all of this should go into comments... somewhere.
Attachment #8923639 - Flags: review?(ted)
Attachment #8923639 - Flags: review?(jcoppeard)
Comment on attachment 8923639 [details] [diff] [review]
Add a --dll flag to the JS shell for loading shared libs,

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

A little crazy but seems like it will work.

::: js/src/jsapi.cpp
@@ +7787,5 @@
> +
> +JS_PUBLIC_API(void)
> +SetBreakpadEnabled(bool enabled)
> +{
> +    static bool* addr = reinterpret_cast<bool*>(dlsym(RTLD_DEFAULT, "gBreakpadInjectorEnabled"));

Does this need to be #ifdef __linux__ or something?

::: js/src/jsapi.h
@@ +7161,5 @@
> + * If the breakpad injector is loaded, prevent it from generating a minidump
> + * for the next crash. Otherwise, do nothing.
> + */
> +extern JS_PUBLIC_API(void)
> +SetBreakpadEnabled(bool enable);

So does the breakpad injector re-enable itself after a crash?  If so maybe the name could reflect the fact that it's only disabled for a single crash.

::: js/src/shell/js.cpp
@@ +8830,5 @@
>  
>      if (op.getBoolOption("no-threads"))
>          js::DisableExtraThreads();
>  
> +    Vector<Library*, 4, SystemAllocPolicy> loadedLibraries;

This chunk could be factored into an AutoLoadLibary RAII class.
Attachment #8923639 - Flags: review?(jcoppeard) → feedback+
(In reply to Jon Coppeard (:jonco) from comment #7)
> ::: js/src/jsapi.cpp
> @@ +7787,5 @@
> > +
> > +JS_PUBLIC_API(void)
> > +SetBreakpadEnabled(bool enabled)
> > +{
> > +    static bool* addr = reinterpret_cast<bool*>(dlsym(RTLD_DEFAULT, "gBreakpadInjectorEnabled"));
> 
> Does this need to be #ifdef __linux__ or something?

Uh, yes, you're right. (I haven't gotten this latest iteration through a try push yet.)

> ::: js/src/jsapi.h
> @@ +7161,5 @@
> > + * If the breakpad injector is loaded, prevent it from generating a minidump
> > + * for the next crash. Otherwise, do nothing.
> > + */
> > +extern JS_PUBLIC_API(void)
> > +SetBreakpadEnabled(bool enable);
> 
> So does the breakpad injector re-enable itself after a crash?  If so maybe
> the name could reflect the fact that it's only disabled for a single crash.

The breakpad injector is part of the process. So after a crash, it's not enabled or disabled; it's gone. The next test will again get --dll passed to it, so it'll start out enabled there.

Oh. My comment sucks. "the next crash" is what is confusing. It should be s/for the next crash/when we crash/. (And this comment was for when I had separate enable/disable functions, so it probably shouldn't be assuming that you're disabling.)

> ::: js/src/shell/js.cpp
> @@ +8830,5 @@
> >  
> >      if (op.getBoolOption("no-threads"))
> >          js::DisableExtraThreads();
> >  
> > +    Vector<Library*, 4, SystemAllocPolicy> loadedLibraries;
> 
> This chunk could be factored into an AutoLoadLibary RAII class.

You're right, that would be much cleaner.
Suppress the minidumps for tests that are expected to crash. (Without this, the tests will fail because they'll get a PROCESS-CRASH and a stack dump, and treeherder or something will interpret that as failure.)
Attachment #8925218 - Flags: review?(jcoppeard)
Ok, posting the patch stack that seems to be fully working on try finally.

ted, both you and jonco have given this r+ in the past, but I added some linux-specific dlsym weirdness that I wanted you to glance at. It'll be used in a later patch, where I need to programmatically turn off minidumps in certain cases before crashing.
Attachment #8925751 - Flags: review?(ted)
Attachment #8923639 - Attachment is obsolete: true
Attachment #8923639 - Flags: review?(ted)
Ok, here's the fully functioning death st... uh, sorry, test harness.

ted: I added instructions on recreating the libbreakpadinjector.so. For linux at least, and that's all this is for, it's a tiny addition to breakpad. But it requires running libtoolize, so the actual changes are huge. You can tell me whether i should try to upstream it or something.

It wouldn't be too hard to make a toolchain job to build this now that it's totally independent of the gecko tree.
Attachment #8925752 - Flags: review?(ted)
Attachment #8925752 - Flags: review?(jcoppeard)
The last missing piece -- crash tests should not trigger the minidump writer, or they will be incorrectly treated as failing.
Attachment #8925753 - Flags: review?(jcoppeard)
Attachment #8925218 - Attachment is obsolete: true
Attachment #8925218 - Flags: review?(jcoppeard)
Attachment #8922094 - Attachment is obsolete: true
Attachment #8920697 - Attachment is obsolete: true
Comment on attachment 8925752 [details] [diff] [review]
When running via autospider.sh, make the shell generate a minidump on crashes

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

r=me for the js/src bits.  Thanks for adding the README and all the comments.

::: js/src/devtools/automation/autospider.py
@@ +451,5 @@
>      jsapi_test_binary = os.path.join(OBJDIR, 'dist', 'bin', 'jsapi-tests')
> +    test_env = env.copy()
> +    if use_minidump and platform.system() == 'Linux':
> +        test_env['LD_PRELOAD'] = injector_lib
> +    st = run_test_command([jsapi_test_binary], env=test_env)

Could use a comment that we use a different mechanism for jsapitests.
Attachment #8925752 - Flags: review?(jcoppeard) → review+
Attachment #8925753 - Flags: review?(jcoppeard) → review+
Comment on attachment 8925751 [details] [diff] [review]
Add a --dll flag to the JS shell for loading shared libs,

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

Overall this is fairly sensible given the constraints you're working with, but if you can do this without adding a public JS API that would be better. If not, and a JS peer is OK with that, then this could be an r+.

::: js/src/jscntxt.cpp
@@ +1660,5 @@
>  void
>  AutoEnterOOMUnsafeRegion::crash(const char* reason)
>  {
>      char msgbuf[1024];
> +    js::SetBreakpadEnabled(false);

Why does this need to be here? It feels a little weird to have this outside of test-specific code.

Also, it looks like this is the only reason you have `SetBreakpadEnabled` as a public API. If you can fix this some other way it would be nice to not expose that quirky implementation detail, and confine it to the shell itself.

::: js/src/shell/js.cpp
@@ +135,5 @@
>  using mozilla::PodEqual;
>  using mozilla::TimeDuration;
>  using mozilla::TimeStamp;
>  
> +// Avoid an unnecessary NSPR dependency on Linux.

This is fine for now since this is just code in the shell and whatnot, but some other possibilities here would be:
1) Add a PR_LoadLibrary etc implementation to the little Posix NSPR shim code we have.
2) Add library-loading code to MFBT.

@@ +137,5 @@
>  using mozilla::TimeStamp;
>  
> +// Avoid an unnecessary NSPR dependency on Linux.
> +#ifdef __linux__
> +typedef void Library;

If you wanted to avoid the #ifdefs below you could also just stick `#define PR_LoadLibrary(l) dlopen(l)` etc. here. Or even `static Library* PR_LoadLibrary(const char* l) { return dlopen(l); }` if you didn't want to resort to preprocessor nonsense.

@@ +3214,5 @@
> +        RootedObject opts(cx, &args[1].toObject());
> +        if (!JS_GetProperty(cx, opts, "suppress_minidump", &v))
> +            return false;
> +        if (v.isBoolean() && v.toBoolean())
> +            js::SetBreakpadEnabled(false);

I don't grok JSAPI very well, so it's possible you might want someone who does to give this a once-over, although it does seem pretty simple.

@@ +8938,5 @@
>          js::DisableExtraThreads();
>  
> +    AutoLibraryLoader loader;
> +    MultiStringRange dllPaths = op.getMultiStringOption("dll");
> +    while (!dllPaths.empty()) {

Does `MultiStringRange` not implement the bits needed for range-based for loops?

@@ +8948,5 @@
> +        // Unhandled Exception Filter and we need to redirect JIT code to it.
> +        // But it's too late to do that now, because the dll disables setting
> +        // the filter to prevent tampering. So look into the DLL to see if has
> +        // the function we want to direct unhandled exceptions to, and if so,
> +        // use it.

This is ...awkward. It's not the *worst* thing, given the constraints, but it might be nice to find something more sensible here.

Is this a problem for JS embeddors that want to have their own exception handling routines as well?
Attachment #8925751 - Flags: review?(ted)
Comment on attachment 8925752 [details] [diff] [review]
When running via autospider.sh, make the shell generate a minidump on crashes

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

Additionally, you should make your spidermonkey task definition default to set the `MINIDUMP_SAVE_PATH` environment variable to your public artifacts directory (if that's not already covered elsewhere) so that minidumps generated in automation get uploaded for developer examination.

::: js/src/devtools/automation/README
@@ +18,5 @@
> +catches crashes and generates minidumps, so that autospider.py can produce a
> +readable stack trace at the end of the run. Currently this library is only
> +available on linux64, and is built via the following procedure:
> +
> +    % git clone https://chromium.googlesource.com/chromium/tools/depot_tools.git

Y'know, since you already wrote this all out, you *could* just put it in shell script form and say "is built by running build-breakpad-injector.sh". :) (If you did so it would also be good to pin revisions of the repositories you're checking out.)

Obviously I'm not going to force that issue, but it'd be a step towards making this a toolchain task which could be built automatically in automation.

::: js/src/devtools/automation/autospider.py
@@ +313,5 @@
>      )
>  
> +if AUTOMATION:
> +    # Currently only supported on linux64.
> +    if platform.system() == 'Linux' and UNAME_M == 'x86_64':

`UNAME_M` sure is a quirky variable name (and also it's actually set by shelling out to `uname -m`, which feels a little silly!) Python does have a `platform.uname` function you could use instead, or just `platform.machine()` here.

@@ +318,5 @@
> +        use_minidump = variant.get('use_minidump', True)
> +    else:
> +        use_minidump = False
> +else:
> +    use_minidump = False

Longer-term I suspect you're going to want to figure out how to let people use this locally as well, but I'm guessing it's not incredibly hard to run JS tests under a debugger so it's probably not a pressing need.

@@ +328,5 @@
> +        env.setdefault('MINIDUMP_STACKWALK',
> +                       os.path.join(DIR.tooltool, 'linux64-minidump_stackwalk'))
> +    elif platform.system() == 'Darwin':
> +        injector_lib = os.path.join(DIR.tooltool, 'breakpad-tools', 'breakpadinjector.dylib')
> +    if not os.path.exists(injector_lib):

You set `injector_lib = None` above, and you don't have an exhaustive list of platforms here, so it's possible that some future code change will wind up with it being `None` here. You can just write `if not (injector_lib and os.path.exists(injector_lib)):`

@@ +372,5 @@
> +        run_command([
> +            'make',
> +            'recurse_syms',
> +            'MOZ_SOURCE_REPO=file://' + DIR.source,
> +            'RUST_TARGET=0', 'RUSTC_COMMIT=0'

You could file a followup to make symbolstore.py do the right thing in standalone JS builds so you don't have to override these.

@@ +545,5 @@
>      subprocess.call(command)
>  
> +# Generate stacks from minidumps.
> +if use_minidump:
> +    venv_python = os.path.join(OBJDIR, "_virtualenv", "bin", "python")

This script is basically mozharness-lite, so it doesn't have access to the Python modules in the objdir virtualenv, eh? This is a little awkward, but I guess fiddling things so you could actually invoke mozcrash as a Python module wouldn't buy you much.

@@ +551,5 @@
> +        venv_python,
> +        os.path.join(DIR.source, "testing/mozbase/mozcrash/mozcrash/mozcrash.py"),
> +        os.getenv("TMPDIR", "/tmp"),
> +        os.path.join(OBJDIR, "dist/crashreporter-symbols"),
> +        "-b", os.getenv("MINIDUMP_STACKWALK")

mozcrash already checks this env var as a default value:
https://dxr.mozilla.org/mozilla-central/rev/2535bad09d720e71a982f3f70dd6925f66ab8ec7/testing/mozbase/mozcrash/mozcrash/mozcrash.py#164
Attachment #8925752 - Flags: review?(ted) → review+
(In reply to Steve Fink [:sfink] [:s:] from comment #11)
> Created attachment 8925752 [details] [diff] [review]
> When running via autospider.sh, make the shell generate a minidump on crashes
> 
> Ok, here's the fully functioning death st... uh, sorry, test harness.
> 
> ted: I added instructions on recreating the libbreakpadinjector.so. For
> linux at least, and that's all this is for, it's a tiny addition to
> breakpad. But it requires running libtoolize, so the actual changes are
> huge. You can tell me whether i should try to upstream it or something.

Yeah, I don't think that's going to make it upstream. :) I explicitly ripped libtool out years ago because I hate it more than any other build tool I've ever encountered. I think it would be simpler to write that tool as a standalone source file that just links against the Breakpad code to generate a shared library. I've done this sort of thing for a number of tools for poking at minidump files in the past, it should be similarly doable for using the client bits:
https://hg.mozilla.org/users/tmielczarek_mozilla.com/dump-lookup/file/39e8632922e6/Makefile

You could just have the Makefile invoke `$(CC) -shared ...` since it's a single-platform thing.

> It wouldn't be too hard to make a toolchain job to build this now that it's
> totally independent of the gecko tree.

That would be a nice improvement just to save you the hassle when someone inevitably says "hey could you also make it do X?", but it can definitely be a followup.
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #14)
> Comment on attachment 8925751 [details] [diff] [review]
> Add a --dll flag to the JS shell for loading shared libs,
> 
> Review of attachment 8925751 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Overall this is fairly sensible given the constraints you're working with,
> but if you can do this without adding a public JS API that would be better.
> If not, and a JS peer is OK with that, then this could be an r+.

I agree that this is unfortunate.

> ::: js/src/jscntxt.cpp
> @@ +1660,5 @@
> >  void
> >  AutoEnterOOMUnsafeRegion::crash(const char* reason)
> >  {
> >      char msgbuf[1024];
> > +    js::SetBreakpadEnabled(false);
> 
> Why does this need to be here? It feels a little weird to have this outside
> of test-specific code.
> 
> Also, it looks like this is the only reason you have `SetBreakpadEnabled` as
> a public API. If you can fix this some other way it would be nice to not
> expose that quirky implementation detail, and confine it to the shell itself.

The problem is that we have tests that are allowed to crash, but only for the specific reason of an OOM that happens to hit in a particular place. And the test harness determines whether that is ok on a per-test basis. So we have a crashing test that should either generate a stack trace or not, depending on where it crashed.

One alternative would be to always generate the minidump, but then have the harness delete it if stderr shows that the crash is "allowed". But that gets a bit messy -- we can't just delete the last generated minidump, because we're running lots of tests in parallel. We could stick in a label, or use a different directory to store the crash for every single test, or something, but that's a lot of plumbing. Also, I expect there will be other sorts of crashes that fit into this category, though I admit that's a bit of a weak argument since I don't have an example. (I can make one up: what if we wanted to test a particular security-critical MOZ_ASSERT, making sure it properly detects whatever it's detecting even when jitting or running in some funky environment or whatever. Then you'd want it to suppress the minidump for that assert and no other.)

So... options seem to be (1) leave as-is with js::SetBreakpadEnable(), (2) do the work to be able to delete the appropriate minidump after the fact, (3) ignore *any* crash in an allow-oom test, or (4) same as #1 but rename it to something more generic like js::ExpectCrash/js::HintCrashExpected, or something more specific like js::SuppressMinidumpOnCrash. #3 is tempting for its simplicity, but there *is* other tooling that externally controls when an OOM will be simulated, and it wants to iterate over the possibilities and ignore the "unhandleable" cases. So it might need to be special-cased in some way (to not use this mechanism at all, or delete the minidumps, or something.)

What do you think makes the most sense?

> ::: js/src/shell/js.cpp
> @@ +135,5 @@
> >  using mozilla::PodEqual;
> >  using mozilla::TimeDuration;
> >  using mozilla::TimeStamp;
> >  
> > +// Avoid an unnecessary NSPR dependency on Linux.
> 
> This is fine for now since this is just code in the shell and whatnot, but
> some other possibilities here would be:
> 1) Add a PR_LoadLibrary etc implementation to the little Posix NSPR shim
> code we have.

I've briefly looked into that, but the hairball of #ifdefs in nspr's dlsym equivalent kind of terrified me.

> @@ +137,5 @@
> >  using mozilla::TimeStamp;
> >  
> > +// Avoid an unnecessary NSPR dependency on Linux.
> > +#ifdef __linux__
> > +typedef void Library;
> 
> If you wanted to avoid the #ifdefs below you could also just stick `#define
> PR_LoadLibrary(l) dlopen(l)` etc. here. Or even `static Library*
> PR_LoadLibrary(const char* l) { return dlopen(l); }` if you didn't want to
> resort to preprocessor nonsense.

Hm, good points.

> @@ +8938,5 @@
> >          js::DisableExtraThreads();
> >  
> > +    AutoLibraryLoader loader;
> > +    MultiStringRange dllPaths = op.getMultiStringOption("dll");
> > +    while (!dllPaths.empty()) {
> 
> Does `MultiStringRange` not implement the bits needed for range-based for
> loops?

No, sadly. It's a range thing (empty()/front()/popFront()) instead of an iterator thing (begin()/end()/operator++()).

> @@ +8948,5 @@
> > +        // Unhandled Exception Filter and we need to redirect JIT code to it.
> > +        // But it's too late to do that now, because the dll disables setting
> > +        // the filter to prevent tampering. So look into the DLL to see if has
> > +        // the function we want to direct unhandled exceptions to, and if so,
> > +        // use it.
> 
> This is ...awkward. It's not the *worst* thing, given the constraints, but
> it might be nice to find something more sensible here.
> 
> Is this a problem for JS embeddors that want to have their own exception
> handling routines as well?

That's a great question. I guess I don't know of any serious Windows embedders offhand. (...maybe this is why...)

As discussed on IRC, I'm just going to drop this part of the patch for now since I only have Linux support at the moment anyway. I'll defer dealing with it to the followup.
Applied review comments, and borrowed the name NoteIntentionalCrash.
Attachment #8927917 - Flags: review?(ted)
Attachment #8925751 - Attachment is obsolete: true
Botched that update by removing a rather critical null pointer check.
Attachment #8927970 - Flags: review?(ted)
Attachment #8927917 - Attachment is obsolete: true
Attachment #8927917 - Flags: review?(ted)
Attachment #8927970 - Flags: review?(ted) → review+
Pushed by sfink@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ec0702d55b01
Add a --dll flag to the JS shell for loading shared libs, r=ted,jonco
https://hg.mozilla.org/integration/mozilla-inbound/rev/5e1dc7fec0ff
When running via autospider.sh, make the shell generate a minidump on crashes, r=jonco,ted
https://hg.mozilla.org/integration/mozilla-inbound/rev/94d20ed3c062
Suppress minidumps for crash tests, r=jonco
Pushed by sfink@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2bb133c24dca
Add a --dll flag to the JS shell for loading shared libs, r=ted,jonco
https://hg.mozilla.org/integration/mozilla-inbound/rev/55903b8dbfbd
When running via autospider.sh, make the shell generate a minidump on crashes, r=jonco,ted
https://hg.mozilla.org/integration/mozilla-inbound/rev/83af876d4fa0
Suppress minidumps for crash tests, r=jonco
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/08115b27691e
followup - Avoid NSPR dependency on OS X too, to unbreak AWFY. r=jonco on IRC
Depends on: 1421159
Followup - MOZ_CRASHREPORTER got lost somewhere. It is necessary to generate the symbols.
Attachment #8937872 - Flags: review?(mh+mozilla)
Comment on attachment 8937872 [details] [diff] [review]
Define MOZ_AUTOMATION so recurse_syms does something

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

The commit message is wrong.
Attachment #8937872 - Flags: review?(mh+mozilla) → review+
Update of earlier patch. This one finally works. Mostly re-requesting review for the MOZ_AUTOMATION_BUILD_SYMBOLS addition; I don't know if there's a better way to do it.

The tricky part was the minidump_stackwalk binary. I guess that's where things went wrong -- I originally was using one that worked, then I switched back to the one from build-tools for consistency (Firefox uses it). That one does not work, I can't figure out why. It complains about not being able to figure out what debug_file is? I just assumed that it would be fine, and didn't verify that it could still display stacks after switching.

Anyway, if I recompile it manually, it works fine. Another argument for setting it up as a toolchain dependency.

With these changes, it's working again: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2d9e76a97b13e5c7c46274d04a8ee74a498b6f68
Attachment #8937920 - Flags: review?(mh+mozilla)
Attachment #8937920 - Flags: review?(mh+mozilla) → review+
The one in build/tools is really old and hasn't kept up with some changes in the minidump format. bug 1378765 covers fixing that.
Blocks: 1426439
Pushed by sfink@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3cfe2c39733f
Define MOZ_CRASHREPORTER and MOZ_AUTOMATION_BUILD_SYMBOLS for recurse_syms, r=glandium
Flags: needinfo?(sphink)
Duplicate of this bug: 1150623
Depends on: 1503316
Duplicate of this bug: 837969
You need to log in before you can comment on or make changes to this bug.