bugzilla.mozilla.org has resumed normal operation. Attachments prior to 2014 will be unavailable for a few days. This is tracked in Bug 1475801.
Please report any other irregularities here.

e10s broken in gtk3 builds

RESOLVED FIXED in mozilla33

Status

()

Core
IPC
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: glandium, Assigned: glandium)

Tracking

unspecified
mozilla33
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

4 years ago
Take a gtk3 build. Open a new e10s window. See that plugin-container doesn't stay up. This comes from the fact that plugin-container is always loaded with the mozgtk2 stub, loading gtk2, while content processes need the same toolkit as the main process, as opposed to plugins. Removing the LD_PRELOAD from ipc/glue/GeckoChildProcessHost.cpp makes the plugin-container process stay up, although it doesn't make the e10s window actually work.

So at the very least, we need to not LD_PRELOAD mozgtk2 for content processes.
(Assignee)

Comment 1

4 years ago
Created attachment 8436414 [details] [diff] [review]
Only load Gtk+2 stub for plugin processes in Gtk+3 builds

With this patch, flash still works, and e10s plugin-container stays up. I guess the reason I get nothing in the e10s window is because of OMTC, so I guess this patch is enough for this bug. The rest will probably be addressed by some of the OMTC bugs.
Attachment #8436414 - Flags: review?(benjamin)
(Assignee)

Updated

4 years ago
Assignee: nobody → mh+mozilla
Status: NEW → ASSIGNED
(Assignee)

Comment 2

4 years ago
Mmmm actually, a gtk2 build has e10s windows working properly. At least they display something. So there is likely another bug hidden in there.
(Assignee)

Comment 3

4 years ago
At least, the patch gets us back to where we were before bug 624422 (tested a gtk3 build pre-624422, with the same behaviour as current m-c + this patch)
The patch looks good to me.
Comment on attachment 8436414 [details] [diff] [review]
Only load Gtk+2 stub for plugin processes in Gtk+3 builds

Review of attachment 8436414 [details] [diff] [review]:
-----------------------------------------------------------------

I think this is generally fine but I'd still get someone who knows gtk better to sign off on it. However:

::: ipc/glue/GeckoChildProcessHost.cpp
@@ +586,5 @@
> +          if (ld_preload && *ld_preload) {
> +              new_ld_preload.AppendLiteral(":");
> +              new_ld_preload.Append(ld_preload);
> +          }
> +          newEnvVars["LD_PRELOAD"] = new_ld_preload.get();

This is dangerous, new_ld_preload doesn't live long enough to guarantee that the buffer is valid when newEnvVars is used.
(Assignee)

Comment 6

4 years ago
(In reply to ben turner [:bent] (use the needinfo? flag!) from comment #5)
> This is dangerous, new_ld_preload doesn't live long enough to guarantee that
> the buffer is valid when newEnvVars is used.

So, in fact, newEnvVars is a std::map<std::string, std::string>, so it is fine (the std::string constructor taking a char * does a copy)
(In reply to Mike Hommey [:glandium] from comment #6)
> So, in fact, newEnvVars is a std::map<std::string, std::string>, so it is
> fine (the std::string constructor taking a char * does a copy)

Ah, indeed. Sorry for the noise.
(Assignee)

Comment 8

4 years ago
Comment on attachment 8436414 [details] [diff] [review]
Only load Gtk+2 stub for plugin processes in Gtk+3 builds

Review of attachment 8436414 [details] [diff] [review]:
-----------------------------------------------------------------

As per irc, taking bent's comment as r+, and adding Karl for the gtk-aware eyes.
Attachment #8436414 - Flags: review?(karlt)
Attachment #8436414 - Flags: review?(benjamin)
Attachment #8436414 - Flags: review+
Attachment #8436414 - Flags: review?(karlt) → review+
https://hg.mozilla.org/mozilla-central/rev/0b68f0f08181
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33

Updated

4 years ago
Blocks: 1034064

Updated

4 years ago
No longer blocks: 1034064
You need to log in before you can comment on or make changes to this bug.