The X11 error handler in GDK2 (used in the plugin process) calls exit(1), without using g_error(), and so this is not caught by Gecko's X/GDK error handler (designed for GDK3) and not triggering a crash report.

This has version of the test manifest from before I realized that crashAndGetCrashServiceRecord is not e10s-compatible: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fbc6fbf785755e94ee612391384ee1bedbbddf9c&selectedJob=69200418

Comment on attachment 8827061 [details] bug 1331320 install X11 error handler through Xlib in plugin process as GTK2 does not use g_error https://reviewboard.mozilla.org/r/104894/#review105782 To be on the safe side, wouldn't it be better to instead change XRE_InstallX11ErrorHandler such that it does a runtime check instead of a compile-time check?

Comment on attachment 8827062 [details] bug 1331320 document requestFlakyTimeout for crashAndGetCrashServiceRecord() https://reviewboard.mozilla.org/r/104896/#review105784

Comment on attachment 8827063 [details] bug 1331320 test that a crashreport is generated on X11 protocol error in plugin https://reviewboard.mozilla.org/r/104898/#review105788 ::: dom/plugins/test/mochitest/mochitest.ini:166 (Diff revision 1) > [test_windowless_flash.html] > skip-if = !(os == "win" && processor == "x86_64") > [test_windowless_ime.html] > skip-if = os != "win" > +[test_x11_error_crash.html] > +skip-if = !crashreporter || e10s || ((toolkit != "gtk2") && (toolkit != "gtk3")) Is it a good idea to skip if e10s is enabled? I'm sure at some point in the future, where e10s is enabled everywhere, we'll start turning off non-e10s tests. That would make this test quietly ignored...

Comment on attachment 8827063 [details] bug 1331320 test that a crashreport is generated on X11 protocol error in plugin https://reviewboard.mozilla.org/r/104898/#review105788 > Is it a good idea to skip if e10s is enabled? I'm sure at some point in the future, where e10s is enabled everywhere, we'll start turning off non-e10s tests. That would make this test quietly ignored... It needs to be skipped with the way crashAndGetCrashServiceRecord() is implemented right now. The other tests using this function are also !e10s only. https://bugzilla.mozilla.org/show_bug.cgi?id=983313#c12 has the reasoning. I'm not clear why the mode of testing was targeted at !e10s. Perhaps crashAndGetCrashServiceRecord will need porting to e10s if tests skipped with e10s are not going to continue to be run !e10s, but I don't think adding another very similar client is going to add much to that porting work.

Comment on attachment 8827061 [details] bug 1331320 install X11 error handler through Xlib in plugin process as GTK2 does not use g_error https://reviewboard.mozilla.org/r/104894/#review105782 I was hoping that XRE_InstallX11ErrorHandler() could go away after MOZ_WIDGET_GTK == 2 is dropped, reducing the level of indirection. I assume XRE_InstallX11ErrorHandler() existed (from bug 517133) in nsEmbedFunctions for the sake of multi-SO/--disable-libxul builds. And I felt a little better not using run-time detection for what was known at compile time. Only the plugin process will have GTK2 and so I guess the safety is about not calling InstallX11ErrorHandler() elsewhere from a GTK3 process. I considered renaming InstallX11ErrorHandler() to InstallGTK2X11ErrorHandler() or similar. Would you prefer that?

I don't have a strong preference. The XRE_ function could just go away and be replaced with the relevant function call in all the places it's used, too.

OK, thanks. It'll be easier to make the XRE function go away once MOZ_WIDGET_GTK == 2 is dropped, and so I'll put that off until then.

Pushed by ktomlinson@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/cee10a2c348a install X11 error handler through Xlib in plugin process as GTK2 does not use g_error r=glandium https://hg.mozilla.org/integration/autoland/rev/b272ced2faa9 document requestFlakyTimeout for crashAndGetCrashServiceRecord() r=glandium https://hg.mozilla.org/integration/autoland/rev/fd57bcd5daf2 test that a crashreport is generated on X11 protocol error in plugin r=glandium

Backed out for frequent X_GetWindowAttributes crashes. https://treeherder.mozilla.org/logviewer.html#?job_id=69485076&repo=autoland https://hg.mozilla.org/integration/autoland/rev/ec799efea60cbc4462ef8cc155b6bdb2a9789e91

For Karl to look at when he's back

Comment on attachment 8832352 [details] bug 1331320 disable e10s browser_tab_dragdrop.js in remaining linux builds https://reviewboard.mozilla.org/r/108674/#review109954

Karl - are we going to land this?

Also please uplift to 53

Pushed by ktomlinson@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/cc259ce5307f install X11 error handler through Xlib in plugin process as GTK2 does not use g_error r=glandium https://hg.mozilla.org/integration/autoland/rev/ef4156c9e06f document requestFlakyTimeout for crashAndGetCrashServiceRecord() r=glandium https://hg.mozilla.org/integration/autoland/rev/507cfcf7b653 test that a crashreport is generated on X11 protocol error in plugin r=glandium https://hg.mozilla.org/integration/autoland/rev/1526bb09ea83 disable e10s browser_tab_dragdrop.js in remaining linux builds r=mccr8

This bug has existed in GTK3 builds since bug 968196 was fixed. GTK3 builds were shipped first in 46. I assume there is no urgent need to uplift to 53?

Mmmmm I think it's worth uplifting to 52 for ESR: it's going to be the first ESR on Gtk+3, and being an ESR, it will be supported longer.

[Tracking Requested - why for this release]: (In reply to Mike Hommey [:glandium] from comment #22) > Mmmmm I think it's worth uplifting to 52 for ESR: it's going to be the first > ESR on Gtk+3, and being an ESR, it will be supported longer. If it turns out this is too big/risky for 52, so be it, but I would imagine it won't be risky.

https://hg.mozilla.org/mozilla-central/rev/cc259ce5307f https://hg.mozilla.org/mozilla-central/rev/ef4156c9e06f https://hg.mozilla.org/mozilla-central/rev/507cfcf7b653 https://hg.mozilla.org/mozilla-central/rev/1526bb09ea83

Please request Aurora/Beta approval on this when you get a chance.

Comment on attachment 8827061 [details] bug 1331320 install X11 error handler through Xlib in plugin process as GTK2 does not use g_error Approval Request Comment Please consider this an approval request for all patches on this bug: the fix and the associated test changes. [Feature/Bug causing the regression]: This bug has existed in GTK3 builds since bug 968196 was fixed. GTK3 builds were shipped first in 46. [User impact if declined]: Some plug-in crashes do not trigger the crash reporter. This includes bug 1237853, which is a plug-in crash that I suspect is happening frequently when e10s is enabled. [Is this code covered by automated tests?]: Yes. [Has the fix been verified in Nightly?]: Yes. [Needs manual test from QE? If yes, steps to reproduce]: No. [List of other uplifts needed for the feature/fix]: None. [Is the change risky?]: No. [Why is the change risky/not risky?]: Affects only the plug-in process, in that it reinstates code that was used for years. [String changes made/needed]: None.

Comment on attachment 8827061 [details] bug 1331320 install X11 error handler through Xlib in plugin process as GTK2 does not use g_error fix xlib error handling in plugin process, aurora53+, beta52+

https://hg.mozilla.org/releases/mozilla-aurora/rev/0711d64f83bd https://hg.mozilla.org/releases/mozilla-aurora/rev/00073c2c7866 https://hg.mozilla.org/releases/mozilla-aurora/rev/158ccc5168b3 https://hg.mozilla.org/releases/mozilla-aurora/rev/2af32640ce76

https://hg.mozilla.org/releases/mozilla-beta/rev/50722ee37c34 https://hg.mozilla.org/releases/mozilla-beta/rev/72eda3f714d5 https://hg.mozilla.org/releases/mozilla-beta/rev/fd7a49fdef4d https://hg.mozilla.org/releases/mozilla-beta/rev/a2d0bdf251a3

https://hg.mozilla.org/releases/mozilla-esr52/rev/50722ee37c34 https://hg.mozilla.org/releases/mozilla-esr52/rev/72eda3f714d5 https://hg.mozilla.org/releases/mozilla-esr52/rev/fd7a49fdef4d https://hg.mozilla.org/releases/mozilla-esr52/rev/a2d0bdf251a3