639725 - plugin-side X11 error messages no longer in crash report app notes

Status: NEW

(Toolkit :: Crash Reporting, defect)

Description

[Karl Tomlinson (:karlt)]

Regression range:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=58101a16aff7&tochange=30239e4cebd8

Looks like
http://hg.mozilla.org/mozilla-central/rev/bcae411bc333

This is currently making it difficult to distinguish regular old fashioned plugin X errors from the wave of GLX errors that has arrived with wmode=direct on
youtube.

Comment 1

[(not currently active) Ted Mielczarek]

Did that ever work? We don't remote AnnotateCrashReport yet (bug 581341).

Comment 2

[Karl Tomlinson (:karlt)]

Yes, this is a regression.  Still working on 1.9.2.

http://hg.mozilla.org/mozilla-central/annotate/5666cb06e519/toolkit/xre/nsX11ErrorHandler.cpp#l151

Comment 3

[(not currently active) Ted Mielczarek]

Ah, I forgot all about that. cjones' changes were necessary because in content processes we run a lot of existing code that tries to do crashreporter things, and we didn't want to have to sprinkle ProcessType checks all over. I think the right solution is just to make the CrashReporter APIs check processtype internally and remote themselves correctly, and switch your annotation there to just use AnnotateCrashReport directly, so that it should work from any type of process.

Comment 4

[Karl Tomlinson (:karlt)]

Thanks for your comments.  It does make sense that we centralize processtype checks.

One issue is that it is harder to remote notes from a non-main thread, and I assume NS_DebugBreak for example could be called from any thread.  (Is CrashReporter::AppendAppNotesToCrashReport() actually safe from a non-main thread?)  I guess we can just have a is-main-thread check and no-op if not.

Something to bear in mind is they we may want something on 2.0, which may be averse to significant changes.  However, with a new release model, I don't know whether we'll continue to support older releases, so this may not end up being an issue.

Comment 5

[(not currently active) Ted Mielczarek]

For a simpler fix, we could perhaps just put the process type checks in all the crashreporter APIs, and bail out for non-chrome processes (except for GetEnabled(), which we could fix to work correctly), so that calling AnnotateCrashReport() would be harmless in content processes. Then this callsite ought to work again, and we can leave remoting the other APIs for that other bug. Does that sound reasonable?

Comment 6

[Karl Tomlinson (:karlt)]

I don't have a feel for how many APIs would need to have the checks added, but that sounds reasonable to me.

Comment 7

[(not currently active) Ted Mielczarek]

http://mxr.mozilla.org/mozilla-central/source/toolkit/crashreporter/nsExceptionHandler.h (as ugly as it has become) is the set of public APIs exposed curerntly.