Closed Bug 1022259 Opened 6 years ago Closed 6 years ago

e10s broken in gtk3 builds

Categories

(Core :: IPC, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: glandium, Assigned: glandium)

References

Details

Attachments

(1 file)

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.
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: nobody → mh+mozilla
Status: NEW → ASSIGNED
Mmmm actually, a gtk2 build has e10s windows working properly. At least they display something. So there is likely another bug hidden in there.
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.
(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.
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
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Blocks: 1034064
No longer blocks: 1034064
You need to log in before you can comment on or make changes to this bug.