Closed Bug 1331320 Opened 3 years ago Closed 3 years ago

install X11 error handler through Xlib in plugin process

Categories

(Toolkit :: Crash Reporting, defect)

46 Branch
All
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox51 --- wontfix
firefox52 + fixed
firefox-esr52 --- fixed
firefox53 --- fixed
firefox54 --- fixed

People

(Reporter: karlt, Assigned: karlt)

References

(Blocks 2 open bugs)

Details

(Keywords: regression)

Attachments

(4 files)

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?
Attachment #8827061 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8827062 [details]
bug 1331320 document requestFlakyTimeout for crashAndGetCrashServiceRecord()

https://reviewboard.mozilla.org/r/104896/#review105784
Attachment #8827062 - Flags: review?(mh+mozilla) → 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...
Attachment #8827063 - Flags: review?(mh+mozilla) → 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

> 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
For Karl to look at when he's back
Flags: needinfo?(karlt)
Comment on attachment 8832352 [details]
bug 1331320 disable e10s browser_tab_dragdrop.js in remaining linux builds

https://reviewboard.mozilla.org/r/108674/#review109954
Attachment #8832352 - Flags: review?(continuation) → review+
Karl - are we going to land this?
Flags: needinfo?(karlt)
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
Blocks: 1333335
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?
Flags: needinfo?(karlt) → needinfo?(rjesup)
Version: Trunk → 46 Branch
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.
Flags: needinfo?(rjesup)
Please request Aurora/Beta approval on this when you get a chance.
Flags: needinfo?(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.
Flags: needinfo?(karlt)
Attachment #8827061 - Flags: approval-mozilla-beta?
Attachment #8827061 - Flags: approval-mozilla-aurora?
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+
Attachment #8827061 - Flags: approval-mozilla-beta?
Attachment #8827061 - Flags: approval-mozilla-beta+
Attachment #8827061 - Flags: approval-mozilla-aurora?
Attachment #8827061 - Flags: approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.