Closed Bug 1332523 Opened 8 years ago Closed 8 years ago

Various cleanup before full fledged refactoring of the bootstrap API

Categories

(Toolkit :: Startup and Profile System, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: glandium, Assigned: glandium)

References

(Blocks 1 open bug)

Details

Attachments

(5 files)

I have some patches that can land independently of more definite changes on the bootstrap API.
Comment on attachment 8828609 [details] Bug 1332523 - Move message indicating when the blocklist is initialized after user32.dll was loaded to the blocklist itself. https://reviewboard.mozilla.org/r/105936/#review106902
Attachment #8828609 - Flags: review?(dmajor) → review+
Comment on attachment 8828610 [details] Bug 1332523 - Make the Bootstrap API entry point the same for both dependent and standalone linkage. https://reviewboard.mozilla.org/r/105938/#review107094 r=me with s/static/inline/ or please rerequest review if that's not possible/good. ::: toolkit/xre/Bootstrap.h:137 (Diff revision 1) > +#else > extern "C" NS_EXPORT void NS_FROZENCALL > XRE_GetBootstrap(Bootstrap::UniquePtr& b); > -typedef void (*GetBootstrapType)(Bootstrap::UniquePtr&); > > +static Bootstrap::UniquePtr static in a header seems bad. Why not just `inline`?
Attachment #8828610 - Flags: review?(benjamin) → review+
Attachment #8828611 - Flags: review?(benjamin) → review+
Comment on attachment 8828612 [details] Bug 1332523 - Make GetBootstrap take the path to an arbitrary file next to libxul. https://reviewboard.mozilla.org/r/105942/#review107098
Attachment #8828612 - Flags: review?(benjamin) → review+
Comment on attachment 8828613 [details] Bug 1332523 - Add BinaryPath::Get variant that returns a UniquePtr instead of filling a stack buffer. https://reviewboard.mozilla.org/r/105944/#review107100 r=me either way with a slight preference for not adding a new FreePolicy class. ::: xpcom/build/BinaryPath.h:158 (Diff revision 1) > > #else > #error Oops, you need platform-specific code here > #endif > > + struct FreePolicy { void operator()(void* p) { free(p); } }; http://searchfox.org/mozilla-central/source/mfbt/UniquePtrExtensions.h#43 already has a FreePolicy implementation. Is it better to repeat that here rather than use the existing one?
Attachment #8828613 - Flags: review?(benjamin) → review+
Heh, I wasn't aware of the UniquePtrExtensions.h header.
Pushed by mh@glandium.org: https://hg.mozilla.org/integration/autoland/rev/beb1258e4871 Move message indicating when the blocklist is initialized after user32.dll was loaded to the blocklist itself. r=dmajor https://hg.mozilla.org/integration/autoland/rev/7402945cf573 Make the Bootstrap API entry point the same for both dependent and standalone linkage. r=bsmedberg https://hg.mozilla.org/integration/autoland/rev/bb2b50d94e76 Remove nsXPCOMGlue.h. r=bsmedberg https://hg.mozilla.org/integration/autoland/rev/e1baf4f6ef96 Make GetBootstrap take the path to an arbitrary file next to libxul. r=bsmedberg https://hg.mozilla.org/integration/autoland/rev/8c3f02820491 Add BinaryPath::Get variant that returns a UniquePtr instead of filling a stack buffer. r=bsmedberg
Yesterday, after this bug had landed, I tried to compile Thunderbird (after integrating the changes here in bug 1332898) with ac_add_options --disable-sandbox in my Mozconfig (which was really an undesired left-over). The result was that TB didn't start, this was the stack: mozglue.dll!arena_dalloc(void * ptr, unsigned __int64 offset) Line 4709 C thunderbird.exe!mozilla::GetBootstrap(const char * aXPCOMFile) Line 417 C++ thunderbird.exe!InitXPCOMGlue(const char * argv0) Line 248 C++ thunderbird.exe!NS_internal_main(int argc, char * * argv, char * * envp) Line 297 C++ thunderbird.exe!wmain(int argc, wchar_t * * argv) Line 118 C++ I debugged it a bit and strangely it failed on the 'return' (!) statement here after successfully returning from (*func)(b): https://dxr.mozilla.org/mozilla-central/rev/3cedab21a7e65e6a1c4c2294ecfb5502575a46e3/xpcom/glue/standalone/nsXPCOMGlue.cpp#407 On the frozen console (couldn't copy/paste) I saw: jemalloc.c:4710: Failed assertion: "arena != NULL" Hit MOZ_CRASH() at jemalloc_config.cpp:163 So something went wrong in memory management. I haven't tried this in Firefox, since it would cost me one hour to compile the potentially failing version and then another hour to return to a good version. However, TB and FF have a very similar main() so I assume it FF wouldn't work either. If anyone is interested, I could build FF, or the interested party could do this. I'm not sure whether this option is still supported, if not, it should be removed instead of causing non-functioning builds.
Flags: needinfo?(mh+mozilla)
Flags: needinfo?(benjamin)
I cannot explain that, but it should be filed and diagnosed in a different bug. It sounds like something that could be figured out with a debugging session.
Flags: needinfo?(mh+mozilla)
Flags: needinfo?(benjamin)
Blocks: 1333081
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: