Closed Bug 1059504 Opened 6 years ago Closed 6 years ago

Fix crashes in plugin-container due to the new v2 bundle structure.

Categories

(Core :: IPC, defect)

All
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla35
Tracking Status
firefox34 --- fixed
firefox35 --- fixed

People

(Reporter: spohl, Assigned: spohl)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

The new v2 bundle structure causes crashes in plugin-container because four libraries cannot be found by dyld. These libraries are referenced as follows in the plugin-container binary:
@executable_path/libmozglue.dylib
@executable_path/libmozalloc.dylib
@executable_path/libnss3.dylib
@executable_path/XUL

I'm not quite sure how this ever worked, since these libraries never lived next to the plugin-container executable. I do know however that our new bundle structure breaks this and causes plugin-container to crash.

Adding a copy of these libraries in plugin-container.app/Contents/MacOS seems like a very bad idea, since this would dramatically increase the size of the bundle.

There are currently two options I can think of:
1. Go back to a single binary, and do something similar to [1] to avoid a dock icon.
2. Find a way to reference the libraries at @executable_path/../../../<libname>

I understand that by some accident, luck or coincidence, we're currently able to load the necessary libraries from Firefox.app/Contents/MacOS. However, this seems to go against the idea of a .app bundle that could be run independently, since it depends on libraries that are at a precise relative path *outside* of the bundle. For this same reason, I don't like option 2, since we'd still have this dependency. We would just make it explicit.

I'm currently strongly in favor of option 1 but would like to ask a few folks for feedback.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=557225#c1
Flags: needinfo?(ted)
Flags: needinfo?(joshmoz)
This is an indication that the glue and dependentlibs.list isn't working correctly, and probably a symptom of further problems. I encourage you to step through the glue-loading code to see whether it's looking in the wrong path for dependentlibs.list or the libraries.
Flags: needinfo?(ted)
Flags: needinfo?(joshmoz)
FWIW, a single binary is something I want to do for all platforms, but not in a short timeframe.
Attached patch Patch (obsolete) — Splinter Review
You're right (no surprise there! :-)). Although dependentlibs.list and the libraries got loaded correctly, I failed to adjust a subsequent lookup of the GREDir. This fixes all the crashes that I've observed and I've also inspected the rest of GeckoChildProcessHost.cpp to ensure that we're doing the proper thing.

This didn't turn out as bad as I thought. I still don't like the idea of referencing libraries that live outside of the .app bundle, but strictly speaking, if we wanted to use the plugin-container.app bundle separately, all we would need to do is add a copy of those four libraries to plugin-container.app/Contents/MacOS.

Thanks for saving me a lot of time, Benjamin!
Comment on attachment 8480295 [details] [diff] [review]
Patch

Try push is almost completely green, so requesting review (updater xpcshell test failures and gaia python integration test suite failures are handled in separate bugs):
https://tbpl.mozilla.org/?tree=Try&rev=7362867ad903
Attachment #8480295 - Flags: review?(benjamin)
Comment on attachment 8480295 [details] [diff] [review]
Patch

I don't think XRE_GetFileFromPath is correct in this context. `path` is not relative nor argv[0]. I'm pretty sure you want NS_NewLocalFile(gGREPath).
Attachment #8480295 - Flags: review?(benjamin) → review-
Attached patch Patch (obsolete) — Splinter Review
Sent this to Oak with all the other patches. Waiting for the results before requesting review again:
https://tbpl.mozilla.org/?tree=Oak&rev=45d91d73dbf5
Attachment #8480295 - Attachment is obsolete: true
Attachment #8488816 - Flags: review?(benjamin)
Comment on attachment 8488816 [details] [diff] [review]
Patch

Not sure you really want `true` for followLinks here. If it works without, false is probably the safer option.
Attachment #8488816 - Flags: review?(benjamin) → review+
Attached patch PatchSplinter Review
Thanks, Benjamin! I wasn't sure which option was safer. Changed followLinks to false.
Attachment #8488816 - Attachment is obsolete: true
Attachment #8489520 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/cbd3f8e4bf49
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Landed on aurora in the Mac V2 signing combined patch in bug 1047584
You need to log in before you can comment on or make changes to this bug.