Karl Tomlinson (:karlt) Assignee
	Description • 7 years ago

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.

	Karl Tomlinson (:karlt) Assignee
	Comment 1 • 7 years ago

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

	Mike Hommey [:glandium]
	Comment 5 • 7 years ago
mozreview-review

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?

	Mike Hommey [:glandium]
	Comment 6 • 7 years ago
mozreview-review

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

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

	Mike Hommey [:glandium]
	Comment 7 • 7 years ago
mozreview-review

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

	Karl Tomlinson (:karlt) Assignee
	Comment 8 • 7 years ago
mozreview-review-reply

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.

	Karl Tomlinson (:karlt) Assignee
	Comment 9 • 7 years ago
mozreview-review-reply

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?

	Mike Hommey [:glandium]
	Comment 10 • 7 years ago

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.

	Karl Tomlinson (:karlt) Assignee
	Comment 11 • 7 years ago

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.

	Pulsebot
	Comment 12 • 7 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 13 • 7 years ago

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

	Randell Jesup [:jesup] (needinfo me)
	Comment 14 • 7 years ago

For Karl to look at when he's back

	Karl Tomlinson (:karlt) Assignee
	Comment 16 • 7 years ago

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

	Andrew McCreight [:mccr8]
	Comment 17 • 7 years ago
mozreview-review

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

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

	Randell Jesup [:jesup] (needinfo me)
	Comment 18 • 7 years ago

Karl - are we going to land this?

	Randell Jesup [:jesup] (needinfo me)
	Comment 19 • 7 years ago

Also please uplift to 53

	Pulsebot
	Comment 20 • 7 years ago

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

	Karl Tomlinson (:karlt) Assignee
	Comment 21 • 7 years ago

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?

	Mike Hommey [:glandium]
	Comment 22 • 7 years ago

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.

	Randell Jesup [:jesup] (needinfo me)
	Comment 23 • 7 years ago

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

	Carsten Book [:Tomcat]
	Comment 24 • 7 years ago
bugherder

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

	Ryan VanderMeulen [:RyanVM]
	Comment 25 • 7 years ago

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

	Karl Tomlinson (:karlt) Assignee
	Comment 26 • 7 years ago

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.

	Julien Cristau [:jcristau]
	Comment 27 • 7 years ago

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+

	Ryan VanderMeulen [:RyanVM]
	Comment 28 • 7 years ago
bugherder uplift

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

	Ryan VanderMeulen [:RyanVM]
	Comment 29 • 7 years ago
bugherder uplift

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

	Ryan VanderMeulen [:RyanVM]
	Comment 30 • 7 years ago
bugherder uplift

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