1331320 - install X11 error handler through Xlib in plugin process

Status: RESOLVED FIXED

(Toolkit :: Crash Reporting, defect)

Description

[Karl Tomlinson (:karlt)]

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.

Comment 1

[Karl Tomlinson (:karlt)]

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 2

[Karl Tomlinson (:karlt)]

Attachment: bug 1331320 install X11 error handler through Xlib in plugin process as GTK2 does not use g_error

Review commit: https://reviewboard.mozilla.org/r/104894/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/104894/

Comment 3

[Karl Tomlinson (:karlt)]

Attachment: bug 1331320 document requestFlakyTimeout for crashAndGetCrashServiceRecord()

Review commit: https://reviewboard.mozilla.org/r/104896/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/104896/

Comment 4

[Karl Tomlinson (:karlt)]

Attachment: bug 1331320 test that a crashreport is generated on X11 protocol error in plugin

Review commit: https://reviewboard.mozilla.org/r/104898/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/104898/

Comment 5

[Mike Hommey [:glandium]]

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 6

[Mike Hommey [:glandium]]

Comment on attachment 8827062 [details]
bug 1331320 document requestFlakyTimeout for crashAndGetCrashServiceRecord()

https://reviewboard.mozilla.org/r/104896/#review105784

Comment 7

[Mike Hommey [:glandium]]

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 8

[Karl Tomlinson (:karlt)]

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 9

[Karl Tomlinson (:karlt)]

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?

Comment 10

[Mike Hommey [:glandium]]

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.

Comment 11

[Karl Tomlinson (:karlt)]

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.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
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

Comment 13

[Ryan VanderMeulen [:RyanVM]]

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

Comment 14

[Randell Jesup [:jesup] (needinfo me)]

For Karl to look at when he's back

Comment 15

[Karl Tomlinson (:karlt)]

Attachment: bug 1331320 disable e10s browser_tab_dragdrop.js in remaining linux builds

The intermittent abnormal plugin exit due to bug 1237853 was not detected by
opt builds due to the use of exit(1) and lack of leak checking (which would
have demonstrated bug 1309420 or bug 1239258).

The abnormal exit is now detected in opt builds also, and so this change
disables testing with these builds also.

Review commit: https://reviewboard.mozilla.org/r/108674/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/108674/

Comment 16

[Karl Tomlinson (:karlt)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3446cbddaf34128a6e57020b8a0949a795402bba

Comment 17

[Andrew McCreight [:mccr8]]

Comment on attachment 8832352 [details]
bug 1331320 disable e10s browser_tab_dragdrop.js in remaining linux builds

https://reviewboard.mozilla.org/r/108674/#review109954

Comment 18

[Randell Jesup [:jesup] (needinfo me)]

Karl - are we going to land this?

Comment 19

[Randell Jesup [:jesup] (needinfo me)]

Also please uplift to 53

Comment 20

[Pulsebot]

Pushed by [e-mail]:
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

Comment 21

[Karl Tomlinson (:karlt)]

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?

Comment 22

[Mike Hommey [:glandium]]

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.

Comment 23

[Randell Jesup [:jesup] (needinfo me)]

[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.

Comment 24

[Carsten Book [:Tomcat]]

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

Comment 25

[Ryan VanderMeulen [:RyanVM]]

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

Comment 26

[Karl Tomlinson (:karlt)]

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 27

[Julien Cristau [:jcristau]]

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+

Comment 28

[Ryan VanderMeulen [:RyanVM]]

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

Comment 29

[Ryan VanderMeulen [:RyanVM]]

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

Comment 30

[Ryan VanderMeulen [:RyanVM]]

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