1332523 - Various cleanup before full fledged refactoring of the bootstrap API

Status: RESOLVED FIXED

(Toolkit :: Startup and Profile System, defect)

Description

[Mike Hommey [:glandium]]

I have some patches that can land independently of more definite changes on the bootstrap API.

Comment 1

[Mike Hommey [:glandium]]

Attachment: Bug 1332523 - Move message indicating when the blocklist is initialized after user32.dll was loaded to the blocklist itself.

Review commit: https://reviewboard.mozilla.org/r/105936/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/105936/

Comment 2

[Mike Hommey [:glandium]]

Attachment: Bug 1332523 - Make the Bootstrap API entry point the same for both dependent and standalone linkage.

Review commit: https://reviewboard.mozilla.org/r/105938/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/105938/

Comment 3

[Mike Hommey [:glandium]]

Attachment: Bug 1332523 - Remove nsXPCOMGlue.h.

Review commit: https://reviewboard.mozilla.org/r/105940/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/105940/

Comment 4

[Mike Hommey [:glandium]]

Attachment: Bug 1332523 - Make GetBootstrap take the path to an arbitrary file next to libxul.

The FileExists check can be removed because the code loading the library
will handle that case already.

Review commit: https://reviewboard.mozilla.org/r/105942/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/105942/

Comment 5

[Mike Hommey [:glandium]]

Attachment: Bug 1332523 - Add BinaryPath::Get variant that returns a UniquePtr instead of filling a stack buffer.

Review commit: https://reviewboard.mozilla.org/r/105944/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/105944/

Comment 6

[(Away)]

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

Comment 7

[Benjamin Smedberg]

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`?

Comment 8

[Benjamin Smedberg]

Comment on attachment 8828611 [details]
Bug 1332523 - Remove nsXPCOMGlue.h.

https://reviewboard.mozilla.org/r/105940/#review107096

Comment 9

[Benjamin Smedberg]

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

Comment 10

[Benjamin Smedberg]

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?

Comment 11

[Mike Hommey [:glandium]]

Heh, I wasn't aware of the UniquePtrExtensions.h header.

Comment 12

[Mike Hommey [:glandium]]

Comment on attachment 8828609 [details]
Bug 1332523 - Move message indicating when the blocklist is initialized after user32.dll was loaded to the blocklist itself.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/105936/diff/1-2/

Comment 13

[Mike Hommey [:glandium]]

Comment on attachment 8828610 [details]
Bug 1332523 - Make the Bootstrap API entry point the same for both dependent and standalone linkage.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/105938/diff/1-2/

Comment 14

[Mike Hommey [:glandium]]

Comment on attachment 8828611 [details]
Bug 1332523 - Remove nsXPCOMGlue.h.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/105940/diff/1-2/

Comment 15

[Mike Hommey [:glandium]]

Comment on attachment 8828612 [details]
Bug 1332523 - Make GetBootstrap take the path to an arbitrary file next to libxul.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/105942/diff/1-2/

Comment 16

[Mike Hommey [:glandium]]

Comment on attachment 8828613 [details]
Bug 1332523 - Add BinaryPath::Get variant that returns a UniquePtr instead of filling a stack buffer.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/105944/diff/1-2/

Comment 17

[Mike Hommey [:glandium]]

Comment on attachment 8828613 [details]
Bug 1332523 - Add BinaryPath::Get variant that returns a UniquePtr instead of filling a stack buffer.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/105944/diff/2-3/

Comment 18

[Pulsebot]

Pushed by [e-mail]:
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

Comment 19

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/beb1258e4871
https://hg.mozilla.org/mozilla-central/rev/7402945cf573
https://hg.mozilla.org/mozilla-central/rev/bb2b50d94e76
https://hg.mozilla.org/mozilla-central/rev/e1baf4f6ef96
https://hg.mozilla.org/mozilla-central/rev/8c3f02820491

Comment 20

[Jorg K (CEST = GMT+2)]

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.

Comment 21

[Benjamin Smedberg]

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.