Closed Bug 1332523 Opened 3 years ago Closed 3 years ago

Various cleanup before full fledged refactoring of the bootstrap API

Categories

(Toolkit :: Startup and Profile System, defect)

defect
Not set

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+
Comment on attachment 8828611 [details]
Bug 1332523 - Remove nsXPCOMGlue.h.

https://reviewboard.mozilla.org/r/105940/#review107096
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.