Closed Bug 1266213 Opened 4 years ago Closed 4 years ago

[e10s] Block users of GTK+ 3.20+ from e10s experiments on Beta

Categories

(Core :: Widget: Gtk, defect)

47 Branch
Unspecified
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
e10s m9+ ---
firefox47 --- fixed
firefox48 --- fixed

People

(Reporter: Felipe, Assigned: Felipe)

References

Details

Attachments

(2 files)

+++ This bug was initially created as a clone of Bug #1264454 +++

There's a new top crasher that affects e10s users of GTK+ 3.20 (bug 1264454).  It's unlikely that it can be fixed before we merge 47 to beta next week (and thus starting a new e10s testing phase). In order to not let that crasher skew the stability numbers during our experiment, it's been suggested to block these users from the experiment.

This is yet to be decided but I wanted to file and write a patch ahead of time since the merge is just a few days away
Attached patch PatchSplinter Review
Attachment #8743541 - Flags: review?(karlt)
Attachment #8743541 - Flags: feedback?(stransky)
Comment on attachment 8743541 [details] [diff] [review]
Patch

The gtk3 test looks sane although I have no idea how the e10s check works in general.
Attachment #8743541 - Flags: feedback?(stransky) → feedback+
Attachment #8743541 - Flags: review?(jmathies)
Note: since this is a very temporary thing, and there's ongoing progress in bug 1264454, I'm inclined to not land the new string here that says "Disabled for GTK..".  If blocked, it will show a standard placeholder text "Unknown" in about:support, which is likely fine for the small amount of users and time that this will be in place.
Comment on attachment 8743541 [details] [diff] [review]
Patch

This appears correct to me based on the docs I found here - 

https://developer.gnome.org/gtk3/stable/gtk3-Feature-Test-Macros.html

I'm assuming we target GTK 3 so we wouldn't be linked to an older library and get an older version of gtk_check_version.
Attachment #8743541 - Flags: review?(jmathies) → review+
I sent to tryserver [1] a version of this patch that doesn't contain the #ifdef RELEASE_BUILD part, in order for this code to take effect.

When the builds are ready [2], could someone who is using GTK 3.20 verify that it works as intended? i.e., e10s was blocked from running. The e10s checkbox in about:preferences should be grayed out.


[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=b6c0a9663bd8
[2] https://archive.mozilla.org/pub/firefox/try-builds/felipc@gmail.com-b6c0a9663bd85c3353c35558cf6fab7592ef3bab/
Johnny said he is running GTK 3.20 and can try it. Johnny, the right build is from Comment 7.  Please run it, go to about:support and check if Multiprocess Windows says "0/0 (Unknown status)".
Flags: needinfo?(jst)
I see:

Multiprocess Windows 	0/1 (Unknown status)
Flags: needinfo?(jst)
Thanks! the patch works as intended to block GTK 3.20. I had said "0/0" but that was a typo.

Try build also passed M-e10s(1) so I think it's safe to say that the test machines are not on 3.20, and we can land the patch without special workarounds for tests.

I'll land it now in order to ask for aurora approval in time for the merge to beta.
Attachment #8743541 - Flags: review?(karlt)
https://hg.mozilla.org/mozilla-central/rev/e08681234156
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Approval Request Comment
[Feature/regressing bug #]: GTK 3.20 API chance that causes a crash
[User impact if declined]: E10s experiment on beta 47 might be crashier than intended if we don't block GTK 3.20 users from it
[Describe test coverage new/current, TreeHerder]: landed on central
[Risks and why]: limited to gtk and the e10s switch
[String/UUID change made/needed]: none
Attachment #8744648 - Flags: review+
Attachment #8744648 - Flags: approval-mozilla-aurora?
I per-emptively pushed this before approval because I want this to make it to the merge, and I've already talked about this patch to ritu.

https://hg.mozilla.org/releases/mozilla-aurora/rev/8551b253f406
Comment on attachment 8744648 [details] [diff] [review]
Patch for aurora, r=jimm

This was pre-approved.
Attachment #8744648 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Blocks: 1268627
Blocks: 1272751
You need to log in before you can comment on or make changes to this bug.