Closed
Bug 1022259
Opened 10 years ago
Closed 10 years ago
e10s broken in gtk3 builds
Categories
(Core :: IPC, defect)
Tracking
()
RESOLVED
FIXED
mozilla33
People
(Reporter: glandium, Assigned: glandium)
References
Details
Attachments
(1 file)
(deleted),
patch
|
karlt
:
review+
glandium
:
review+
|
Details | Diff | Splinter Review |
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•10 years ago
|
||
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•10 years ago
|
Assignee: nobody → mh+mozilla
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•10 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•10 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)
Comment 4•10 years ago
|
||
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•10 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•10 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+
Updated•10 years ago
|
Attachment #8436414 -
Flags: review?(karlt) → review+
Assignee | ||
Comment 9•10 years ago
|
||
Comment 10•10 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in
before you can comment on or make changes to this bug.
Description
•