1471532 - Support Windows in ASan Nightly Reporter builds

Status: RESOLVED FIXED

(Firefox :: General, enhancement)

Description

[Christian Holler (:decoder)]

Currently, the ASan Nightly Reporter builds only work on Linux because they hardcode the ASan "log_path" option to point to the "/tmp" directory. We previously thought that this was the only option, but apparently there is an internal ASan API to change the log_path even at runtime:

> void __sanitizer_set_report_path(const char *path);

Using this method, we should be able to set the log_path to a directory contained in the profile directory, making the approach work cross-platform. The tricky part here is that we need to set this in every process we start, including every child process (as early as possible). There is currently no generic way to pass the profile directory to all child processes and there are two options for achieving this:

1) Add -profile to the arguments for every child process and implement the handling in every child process class.

2) Export the desired path to an environment variable in the parent, then generically retrieve/set it in XRE_InitChildProcess.

I found that 2) is the more robust solution because 1) not only requires changes to a lot of additional code but also adds additional risk of forgetting to set the proper handling in a certain process type (esp. if more process types are added). It feels much safer to have this as early as possible in one central location instead, that all childs must go through.

I have a patch for this that I tested on Linux. However, I would want someone to test this on Windows before review.

Comment 1

[Christian Holler (:decoder)]

Attachment: Bug 1471532 - Support Windows in ASan Nightly Reporter builds.

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

Comment 2

[Christian Holler (:decoder)]

(In reply to Christian Holler (:decoder) from comment #1)
> Created attachment 8988244 [details]
> Bug 1471532 - Support Windows in ASan Nightly Reporter builds.
> 
> Review commit: https://reviewboard.mozilla.org/r/253474/diff/#index_header
> See other reviews: https://reviewboard.mozilla.org/r/253474/

This patch was tested and confirmed to work on Linux and Windows (the latter with --disable-sandbox, because on Windows ASan, sandboxing is still enabled). I will disable sandboxing in the Windows ASan Reporter Build once we are making that.

Comment 3

[Nathan Froyd [:froydnj]]

Comment on attachment 8988244 [details]
Bug 1471532 - Support Windows in ASan Nightly Reporter builds.

https://reviewboard.mozilla.org/r/253474/#review260466

WFM, just some questions that need to be answered first.

::: browser/extensions/asan-reporter/bootstrap.js:67
(Diff revision 1)
> -  // We could use OS.Constants.Path.tmpDir here, but unfortunately there is
> +  processDirectory(asanDumpDir);
> -  // no way in C++ to get the same value *prior* to xpcom initialization.
> -  // Since ASan needs its options, including the "log_path" option already
> -  // at early startup, there is no way to pass this on to ASan.
> -  //
> -  // Instead, we hardcode the /tmp directory here, which should be fine in
> -  // most cases, as long as we are on Linux and Mac (the main targets for
> -  // this addon at the time of writing).

Just to be clear: choosing the profile directory now means that asan errors prior to setting the appropriate directory will not be reported?

::: toolkit/xre/nsAppRunner.cpp:5338
(Diff revision 1)
> +  std::string path(nspath.get(), nspath.Length());
> +
> +  __sanitizer_set_report_path(path.c_str());

It seems pretty strange to construct a `std::string` just to pass its data as `.c_str()`.  Why not just use `nspath.get()` here?

::: toolkit/xre/nsEmbedFunctions.cpp:374
(Diff revision 1)
> +  nsCOMPtr<nsIFile> asanReporterPath = GetFileFromEnv("ASAN_REPORTER_PATH");
> +  if (!asanReporterPath) {
> +    MOZ_CRASH("Child did not receive ASAN_REPORTER_PATH!");
> +  }
> +  setASanReporterPath(asanReporterPath);

Does this path also need to be set for other processes (e.g. the GPU process, webextension process, etc.)?

Comment 4

[Christian Holler (:decoder)]

(In reply to Nathan Froyd [:froydnj] from comment #3)

Summarizing what we discussed on IRC:

> 
> Just to be clear: choosing the profile directory now means that asan errors
> prior to setting the appropriate directory will not be reported?

Yes. Any errors happening before calling setASanReporterPath will be dumped to stderr and we won't be able to collect them. Which is why it is important to set this in every process as early as possible (but in order to be platform independent, we have to wait until we have established a common writable location). This is very similar to what our regular crash reporter does.

> 
> ::: toolkit/xre/nsAppRunner.cpp:5338
> (Diff revision 1)
> > +  std::string path(nspath.get(), nspath.Length());
> > +
> > +  __sanitizer_set_report_path(path.c_str());
> 
> It seems pretty strange to construct a `std::string` just to pass its data
> as `.c_str()`.  Why not just use `nspath.get()` here?
> 

I will fix this in the next review push, this is just leftovers from refactoring.

> ::: toolkit/xre/nsEmbedFunctions.cpp:374
> (Diff revision 1)
> > +  nsCOMPtr<nsIFile> asanReporterPath = GetFileFromEnv("ASAN_REPORTER_PATH");
> > +  if (!asanReporterPath) {
> > +    MOZ_CRASH("Child did not receive ASAN_REPORTER_PATH!");
> > +  }
> > +  setASanReporterPath(asanReporterPath);
> 
> Does this path also need to be set for other processes (e.g. the GPU
> process, webextension process, etc.)?


Yes, this needs to be set for all processes. However, it is my understanding that XRE_InitChildProcess is called for all of these process types, not just the content child process.

Comment 5

[Christian Holler (:decoder)]

Comment on attachment 8988244 [details]
Bug 1471532 - Support Windows in ASan Nightly Reporter builds.

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

Comment 6

[Nathan Froyd [:froydnj]]

Comment on attachment 8988244 [details]
Bug 1471532 - Support Windows in ASan Nightly Reporter builds.

https://reviewboard.mozilla.org/r/253474/#review260516

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/64f3290fac6e
Support Windows in ASan Nightly Reporter builds. r=froydnj

Comment 8

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/64f3290fac6e

Comment 9

[Cristina Coroiu [:ccoroiu]]

Backed out changeset 64f3290fac6e (bug 1471532) on reqest by decode

Backout: https://hg.mozilla.org/mozilla-central/rev/28ad9a9e95d518e1163e550ae19c972aabb44df5

Comment 10

[Christian Holler (:decoder)]

This caused a perma-red on ASan Reporter builds due to the fact that xpcshell uses a different entrypoint (XRE_XPCShellMain), which didn't set the ASAN_REPORTER_PATH environment variable. During a try run, I noticed a similar problem with a gtest for media/cdm.

For xpcshell, the best approach seems to be to entirely disable the reporter logic when we go through that entrypoint. The other failure I have yet to investigate. I assume these are all test-only failures though.

Comment 11

[Christian Holler (:decoder)]

Comment on attachment 8988244 [details]
Bug 1471532 - Support Windows in ASan Nightly Reporter builds.

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

Comment 12

[Christian Holler (:decoder)]

(In reply to Christian Holler (:decoder) from comment #11)
> Comment on attachment 8988244 [details]
> Bug 1471532 - Support Windows in ASan Nightly Reporter builds.
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/253474/diff/2-3/

I've reopened the review and pushed additional changes that fix the perma-red that occurred in bug 1472154 as well as a crash I saw when running gtests locally. Both failures were related to the fact that childs were spawned though different code paths (xpcshell and gtests in these cases) for testing purposes only.

Let me know if this looks good to you.

Comment 13

[Nathan Froyd [:froydnj]]

Comment on attachment 8988244 [details]
Bug 1471532 - Support Windows in ASan Nightly Reporter builds.

https://reviewboard.mozilla.org/r/253474/#review261024

::: toolkit/xre/nsAppRunner.cpp
(Diff revisions 2 - 3)
>      status = kE10sForceDisabled;
>    }
>  
>    gBrowserTabsRemoteStatus = status;
>  
> -  mozilla::Telemetry::Accumulate(mozilla::Telemetry::E10S_STATUS, status);

I'm going to assume this was done due to rebasing your commits?

Comment 14

[Nathan Froyd [:froydnj]]

(In reply to Christian Holler (:decoder) from comment #12)
> I've reopened the review and pushed additional changes that fix the
> perma-red that occurred in bug 1472154 as well as a crash I saw when running
> gtests locally. Both failures were related to the fact that childs were
> spawned though different code paths (xpcshell and gtests in these cases) for
> testing purposes only.
> 
> Let me know if this looks good to you.

Sure, thanks.

Comment 15

[Christian Holler (:decoder)]

Comment on attachment 8988244 [details]
Bug 1471532 - Support Windows in ASan Nightly Reporter builds.

https://reviewboard.mozilla.org/r/253474/#review261024

Yes, this is due to the rebase and appears to be a weirdness in reviewboard, because the original raw patch doesn't contain this.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0ba1353599d5
Support Windows in ASan Nightly Reporter builds. r=froydnj

Comment 17

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/0ba1353599d5