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

NEW
Unassigned

Status

()

8 years ago
7 years ago

People

(Reporter: karlt, Unassigned)

Tracking

Trunk
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Reporter)

Description

8 years ago
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.
Did that ever work? We don't remote AnnotateCrashReport yet (bug 581341).
(Reporter)

Comment 2

8 years ago
Yes, this is a regression.  Still working on 1.9.2.

http://hg.mozilla.org/mozilla-central/annotate/5666cb06e519/toolkit/xre/nsX11ErrorHandler.cpp#l151
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.
(Reporter)

Comment 4

8 years ago
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.
Depends on: 581341
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?
(Reporter)

Comment 6

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

Updated

8 years ago
Blocks: 640629
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.

Updated

8 years ago
No longer blocks: 640629
(Reporter)

Updated

7 years ago
Blocks: 671916
You need to log in before you can comment on or make changes to this bug.