1022259 - e10s broken in gtk3 builds

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Mike Hommey [:glandium]]

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.

Comment 1

[Mike Hommey [:glandium]]

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

Comment 2

[Mike Hommey [:glandium]]

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

Comment 3

[Mike Hommey [:glandium]]

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)

Comment 4

[Emilio Pozuelo Monfort]

The patch looks good to me.

Comment 5

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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.

Comment 6

[Mike Hommey [:glandium]]

(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)

Comment 7

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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

Comment 8

[Mike Hommey [:glandium]]

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.

Comment 9

[Mike Hommey [:glandium]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0b68f0f08181

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/0b68f0f08181