Bug 192405 - [WTF] Add environment variable helpers
Summary: [WTF] Add environment variable helpers
Status: REOPENED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ross Kirsling
URL:
Keywords: InRadar
Depends on: 194710
Blocks:
  Show dependency treegraph
 
Reported: 2018-12-05 09:30 PST by Don Olmstead
Modified: 2019-02-22 11:10 PST (History)
16 users (show)

See Also:


Attachments
Proof of concept (29.02 KB, patch)
2019-02-06 18:00 PST, Ross Kirsling
no flags Details | Formatted Diff | Diff
Proof of concept (29.57 KB, patch)
2019-02-06 18:10 PST, Ross Kirsling
no flags Details | Formatted Diff | Diff
Checkpoint for EWS (57.24 KB, patch)
2019-02-07 17:57 PST, Ross Kirsling
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews202 for win-future (392.63 KB, application/zip)
2019-02-07 20:39 PST, Build Bot
no flags Details
Checkpoint for EWS (107.94 KB, patch)
2019-02-08 18:01 PST, Ross Kirsling
no flags Details | Formatted Diff | Diff
Checkpoint for EWS (120.38 KB, patch)
2019-02-08 21:30 PST, Ross Kirsling
no flags Details | Formatted Diff | Diff
Checkpoint for EWS (120.44 KB, patch)
2019-02-08 21:53 PST, Ross Kirsling
no flags Details | Formatted Diff | Diff
Checkpoint for EWS (120.56 KB, patch)
2019-02-08 22:12 PST, Ross Kirsling
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews107 for mac-highsierra-wk2 (2.65 MB, application/zip)
2019-02-08 23:31 PST, Build Bot
no flags Details
All but the ASSERT (120.47 KB, patch)
2019-02-08 23:41 PST, Ross Kirsling
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews104 for mac-highsierra-wk2 (3.02 MB, application/zip)
2019-02-09 00:38 PST, Build Bot
no flags Details
All but the ASSERT (120.77 KB, patch)
2019-02-09 15:27 PST, Ross Kirsling
no flags Details | Formatted Diff | Diff
All but the ASSERT (121.02 KB, patch)
2019-02-09 18:17 PST, Ross Kirsling
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews104 for mac-highsierra-wk2 (2.56 MB, application/zip)
2019-02-09 19:34 PST, Build Bot
no flags Details
Patch (123.96 KB, patch)
2019-02-10 18:57 PST, Ross Kirsling
no flags Details | Formatted Diff | Diff
Patch (121.03 KB, patch)
2019-02-11 17:17 PST, Ross Kirsling
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews113 for mac-highsierra (2.12 MB, application/zip)
2019-02-11 20:14 PST, Build Bot
no flags Details
Patch (122.42 KB, patch)
2019-02-11 20:19 PST, Ross Kirsling
no flags Details | Formatted Diff | Diff
Patch (122.91 KB, patch)
2019-02-11 21:29 PST, Ross Kirsling
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews117 for mac-highsierra (2.12 MB, application/zip)
2019-02-11 23:27 PST, Build Bot
no flags Details
Patch (121.21 KB, patch)
2019-02-12 11:27 PST, Ross Kirsling
no flags Details | Formatted Diff | Diff
Patch (121.00 KB, patch)
2019-02-12 14:22 PST, Ross Kirsling
no flags Details | Formatted Diff | Diff
Patch (121.00 KB, patch)
2019-02-13 13:36 PST, Ross Kirsling
no flags Details | Formatted Diff | Diff
Patch (121.02 KB, patch)
2019-02-13 13:59 PST, Ross Kirsling
no flags Details | Formatted Diff | Diff
Patch (119.52 KB, patch)
2019-02-13 15:07 PST, Ross Kirsling
no flags Details | Formatted Diff | Diff
Patch for landing (111.32 KB, patch)
2019-02-14 10:53 PST, Ross Kirsling
no flags Details | Formatted Diff | Diff
Patch for landing (119.70 KB, patch)
2019-02-14 10:55 PST, Ross Kirsling
no flags Details | Formatted Diff | Diff
Patch for landing (119.53 KB, patch)
2019-02-14 11:25 PST, Ross Kirsling
no flags Details | Formatted Diff | Diff
Patch for landing (109.25 KB, patch)
2019-02-15 13:50 PST, Ross Kirsling
no flags Details | Formatted Diff | Diff
Patch for landing (117.62 KB, patch)
2019-02-15 14:08 PST, Ross Kirsling
no flags Details | Formatted Diff | Diff
Patch (117.57 KB, patch)
2019-02-19 10:59 PST, Ross Kirsling
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Don Olmstead 2018-12-05 09:30:18 PST
Currently there are a lot of getenv()/setenv() variables peppered throughout the codebase. It would be nice to have helper functions that would be able to wrap these calls and also convert to and from different types.

Also there are fun problems with the thread safety of these calls, https://blogs.gnome.org/mcatanzaro/2018/02/28/on-setenv-and-explosions/ , so this could be made to provide a bit more safety.
Comment 1 Michael Catanzaro 2018-12-05 09:31:58 PST
Something will always be calling raw getenv() in secondary threads. Any safety will be an illusion at best.
Comment 2 Don Olmstead 2018-12-05 09:40:25 PST
(In reply to Michael Catanzaro from comment #1)
> Something will always be calling raw getenv() in secondary threads. Any
> safety will be an illusion at best.

Well either way it would be nice to have a way to get back values in a nice way and avoid parsing strings.

https://github.com/WebKit/webkit/blob/master/Source/WebCore/platform/network/curl/CurlContext.cpp#L50-L72

Also there's inconsistency with the use of g_getenv and getenv.
Comment 3 Fujii Hironori 2018-12-05 19:02:23 PST
Accounting to the glibc document, it is safe to call getenv in multiple threads, but not safe with other libc.

https://www.gnu.org/software/libc/manual/html_node/Environment-Access.html

> This function returns a string that is the value of the
> environment variable name. You must not modify this string. In
> some non-Unix systems not using the GNU C Library, it might be
> overwritten by subsequent calls to getenv (but not by any other
> library function). If the environment variable name is not
> defined, the value is a null pointer.


glib document also mentions the next g_getenv call may overrite the previous buffer.

https://developer.gnome.org/glib/stable/glib-Miscellaneous-Utility-Functions.html#g-getenv

> the value of the environment variable, or NULL if the environment
> variable is not found. The returned string may be overwritten by
> the next call to g_getenv(), g_setenv() or g_unsetenv().

Following links are also interesting.

0000188: thread-safe getenv() - Austin Group Defect Tracker
http://austingroupbugs.net/view.php?id=188

Bug 659326 – implement g_app_launch_context_get_environment for win32
https://bugzilla.gnome.org/show_bug.cgi?id=659326

I think we shouldn't assume glibc's behavior. 
We should avoid using getenv in WebKit.
Comment 4 Fujii Hironori 2018-12-06 05:22:40 PST
getenv, getenv_s - cppreference.com
https://en.cppreference.com/w/c/program/getenv

> This function is not required to be thread-safe. Another call to
> getenv, as well as a call to the POSIX functions setenv(),
> unsetenv(), and putenv() may invalidate the pointer returned by a
> previous call or modify the string obtained from a previous call.

I didn't know getenv_s!
Comment 5 Michael Catanzaro 2018-12-06 12:24:27 PST
(In reply to Fujii Hironori from comment #3)
> I think we shouldn't assume glibc's behavior. 
> We should avoid using getenv in WebKit.

Yeah I think you're right, at least for cross-platform code. This is easier said than done, since no doubt loads of our dependencies use gettext or other things that use getenv(), and probably in threads. :(

For WPE/GTK it would be totally infeasible and we will have to rely on glibc's safety assumptions. :/
Comment 6 Ross Kirsling 2019-02-06 18:00:06 PST
Created attachment 361353 [details]
Proof of concept
Comment 7 Build Bot 2019-02-06 18:02:27 PST Comment hidden (obsolete)
Comment 8 Ross Kirsling 2019-02-06 18:03:56 PST
Attached a proof of concept (i.e. WTF and JSC only) of a WTF::Environment, based on discussion with Don.

Let me know if any course corrections should be made.
If we like this approach, then I can carry it out through the rest of Source/ and Tools/.
Comment 9 Ross Kirsling 2019-02-06 18:10:35 PST
Created attachment 361357 [details]
Proof of concept
Comment 10 Build Bot 2019-02-06 18:13:18 PST Comment hidden (obsolete)
Comment 11 Michael Catanzaro 2019-02-06 18:45:23 PST
Comment on attachment 361357 [details]
Proof of concept

View in context: https://bugs.webkit.org/attachment.cgi?id=361357&action=review

I like Environment::get.

Environment::set, ::add, and ::remove are all extremely dangerous, but I'm not opposed to adding them, as long as we make them hard to misuse. They should somehow ASSERT that WTF::initializeThreading has not yet been called. Even that is imperfect -- since threads might have been created outside WTF -- but that should avoid most potential misuse.

Grepping Source/ for usage of setenv and unsetenv, I see mostly just bugs. There is unsafe use in DatabaseManagerCocoa.mm, in WebKitLegacy's WebView.mm, in WebProcessPoolGtk.cpp and WebProcessPoolWPE.cpp, and -- not sure -- but possibly also in PluginProcessMac.mm and AuxiliaryProcessMac.mm. So if developers try to use our safer versions that ASSERT() against misuse, that could be beneficial. (Of course, developers will still use [un]setenv directly, but we could write a style checker rule to prevent that.)

> Source/JavaScriptCore/inspector/remote/glib/RemoteInspectorGlib.cpp:78
> -        [](GObject*, GAsyncResult* result, gpointer userData) {
> +        [&](GObject*, GAsyncResult* result, gpointer userData) {

Looks like stack use after scope, right? You have captured inspectorServerString by reference, but it's a local variable in the parent scope, so it will be disaster when the lambda is called. Got to be really careful about capture by reference. We also prefer to explicitly capture just the variables you need:

[inspectorServerString](GObject* ...) {

> Source/WTF/wtf/PlatformWPE.cmake:19
> +    glib/EnvironmentGlib.cpp

I know existing files are inconsistent, but when adding new ones, let's use GLib with a capital L. Eventually I'll get bored and correct older files.

But honestly, the GLib implementations seem low-value. We could probably just rename EnvironmentGeneric.cpp to Environment.cpp and use that. Looking at the implementations, g_getenv() just calls getenv() and g_setenv() just calls setenv(). (g_setenv() also contains a workaround for ancient systems that don't have setenv(), including a comment that points out it is leaking memory. We don't care.) So I'd remove the GLib implementation and use the generic one always.

> Source/WTF/wtf/glib/EnvironmentGlib.cpp:40
> +    g_setenv(variable.utf8().data(), value.utf8().data(), 1);

But if we were to keep the GLib implementation, we should pass TRUE for the third parameter, since it is a gboolean, unlike setenv. And FALSE down below in the add function. (But let's just remove it.)

> Source/WTF/wtf/glib/EnvironmentGlib.cpp:46
> +void add(const String& variable, const String& value)
> +{
> +    g_setenv(variable.utf8().data(), value.utf8().data(), 0);
> +}

Problem is the name add() doesn't adequately represent what this function does. It means "set the environment variable if it does not already exist, and do nothing otherwise." What would be a better name to express that?
Comment 12 Michael Catanzaro 2019-02-06 18:46:35 PST
Could also merge the WebKit-layer EnvironmentUtilities.cpp into this, to ensure that dangerous function is not misused either.
Comment 13 Michael Catanzaro 2019-02-06 18:54:37 PST
(In reply to Michael Catanzaro from comment #11)
> in WebProcessPoolGtk.cpp and WebProcessPoolWPE.cpp

Reported bug #194370
Comment 14 Ross Kirsling 2019-02-06 19:34:19 PST
Thanks for the thorough review!

(In reply to Michael Catanzaro from comment #11)
> I like Environment::get.
> 
> Environment::set, ::add, and ::remove are all extremely dangerous, but I'm
> not opposed to adding them, as long as we make them hard to misuse. They
> should somehow ASSERT that WTF::initializeThreading has not yet been called.
> Even that is imperfect -- since threads might have been created outside WTF
> -- but that should avoid most potential misuse.
> 
> Grepping Source/ for usage of setenv and unsetenv, I see mostly just bugs.
> There is unsafe use in DatabaseManagerCocoa.mm, in WebKitLegacy's
> WebView.mm, in WebProcessPoolGtk.cpp and WebProcessPoolWPE.cpp, and -- not
> sure -- but possibly also in PluginProcessMac.mm and AuxiliaryProcessMac.mm.
> So if developers try to use our safer versions that ASSERT() against misuse,
> that could be beneficial. (Of course, developers will still use [un]setenv
> directly, but we could write a style checker rule to prevent that.)

Cool, I'll see what I can do!

> > Source/JavaScriptCore/inspector/remote/glib/RemoteInspectorGlib.cpp:78
> > -        [](GObject*, GAsyncResult* result, gpointer userData) {
> > +        [&](GObject*, GAsyncResult* result, gpointer userData) {
> 
> Looks like stack use after scope, right? You have captured
> inspectorServerString by reference, but it's a local variable in the parent
> scope, so it will be disaster when the lambda is called. Got to be really
> careful about capture by reference. We also prefer to explicitly capture
> just the variables you need:
> 
> [inspectorServerString](GObject* ...) {

Derp, this was totally "last thing before I run to dinner" code...

> ... I'd remove the GLib implementation and use the generic one always.

Sounds good to me!

> > Source/WTF/wtf/glib/EnvironmentGlib.cpp:46
> > +void add(const String& variable, const String& value)
> > +{
> > +    g_setenv(variable.utf8().data(), value.utf8().data(), 0);
> > +}
> 
> Problem is the name add() doesn't adequately represent what this function
> does. It means "set the environment variable if it does not already exist,
> and do nothing otherwise." What would be a better name to express that?

I actually stole this name from HashMap because I couldn't think of a better one:
https://github.com/WebKit/webkit/blob/master/Source/WTF/wtf/HashMap.h#L118-L128

Something like "setIfEmpty" could also work though.
Comment 15 Ross Kirsling 2019-02-06 19:59:03 PST
Wowsers, it was not at all obvious to me at a glance, but Windows doesn't have setenv or unsetenv! That said, apparently the only Windows code (outside of Source/ThirdParty) that's modifying env vars is DLLLauncher (by means of SetEnvironmentVariable). So I suppose we could just #if out set/add/remove on Windows for now?
Comment 16 Michael Catanzaro 2019-02-07 07:58:48 PST
Yeah. Probably best to remove the the functions entirely, not just their implementations, to ensure nobody can call them except inside #!OS(WINDOWS) guards.
Comment 17 Ross Kirsling 2019-02-07 17:57:12 PST
Created attachment 361478 [details]
Checkpoint for EWS
Comment 18 Build Bot 2019-02-07 17:59:53 PST Comment hidden (obsolete)
Comment 19 Build Bot 2019-02-07 20:39:42 PST Comment hidden (obsolete)
Comment 20 Build Bot 2019-02-07 20:39:44 PST Comment hidden (obsolete)
Comment 21 Ross Kirsling 2019-02-08 18:01:50 PST
Created attachment 361574 [details]
Checkpoint for EWS
Comment 22 Ross Kirsling 2019-02-08 21:30:16 PST
Created attachment 361592 [details]
Checkpoint for EWS
Comment 23 Build Bot 2019-02-08 21:32:36 PST Comment hidden (obsolete)
Comment 24 Ross Kirsling 2019-02-08 21:53:14 PST
Created attachment 361594 [details]
Checkpoint for EWS
Comment 25 Build Bot 2019-02-08 21:56:13 PST Comment hidden (obsolete)
Comment 26 Ross Kirsling 2019-02-08 22:12:31 PST
Created attachment 361595 [details]
Checkpoint for EWS
Comment 27 Build Bot 2019-02-08 22:14:39 PST Comment hidden (obsolete)
Comment 28 Build Bot 2019-02-08 23:31:14 PST Comment hidden (obsolete)
Comment 29 Build Bot 2019-02-08 23:31:16 PST Comment hidden (obsolete)
Comment 30 Ross Kirsling 2019-02-08 23:41:06 PST
Created attachment 361600 [details]
All but the ASSERT
Comment 31 Build Bot 2019-02-08 23:43:27 PST Comment hidden (obsolete)
Comment 32 Build Bot 2019-02-09 00:38:03 PST Comment hidden (obsolete)
Comment 33 Build Bot 2019-02-09 00:38:05 PST Comment hidden (obsolete)
Comment 34 Ross Kirsling 2019-02-09 15:27:26 PST
Created attachment 361613 [details]
All but the ASSERT
Comment 35 Build Bot 2019-02-09 15:30:59 PST Comment hidden (obsolete)
Comment 36 Ross Kirsling 2019-02-09 18:17:09 PST
Created attachment 361618 [details]
All but the ASSERT
Comment 37 Build Bot 2019-02-09 18:19:32 PST Comment hidden (obsolete)
Comment 38 Build Bot 2019-02-09 19:34:36 PST Comment hidden (obsolete)
Comment 39 Build Bot 2019-02-09 19:34:38 PST Comment hidden (obsolete)
Comment 40 Ross Kirsling 2019-02-10 18:57:12 PST
Created attachment 361653 [details]
Patch
Comment 41 Build Bot 2019-02-10 19:00:23 PST
Attachment 361653 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/posix/EnvironmentPOSIX.cpp:27:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WTF/wtf/win/EnvironmentWin.cpp:27:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WTF/wtf/DataLog.cpp:29:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 3 in 88 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 42 Ross Kirsling 2019-02-10 22:49:05 PST
A couple of notes:

1. The refactor of TestController::libraryPathForTesting() might seem a bit more than necessary, but I was noticing locally that the doubling of String::fromUTF8 seemed to be causing failures.

(In reply to Michael Catanzaro from comment #12)
> Could also merge the WebKit-layer EnvironmentUtilities.cpp into this, to
> ensure that dangerous function is not misused either.

2. I'd prefer to postpone this one to a later patch, since the API test itself would hit the ASSERT. :(

3. Here's hoping my estimation of which g_[un]setenv calls would hit the ASSERT is correct (in particular, in WebKitGLib/TestAutomationSession, I think the beforeAll() seems safe but the afterAll() probably isn't).
Comment 43 Michael Catanzaro 2019-02-11 07:09:34 PST
(In reply to Ross Kirsling from comment #42)
> 2. I'd prefer to postpone this one to a later patch, since the API test
> itself would hit the ASSERT. :(

So that means the API test is wrong (at risk of randomly crashing).

setenv() is *really* tricky! (See bug #194370.)
Comment 44 Ross Kirsling 2019-02-11 10:45:17 PST
(In reply to Michael Catanzaro from comment #43)
> (In reply to Ross Kirsling from comment #42)
> > 2. I'd prefer to postpone this one to a later patch, since the API test
> > itself would hit the ASSERT. :(
> 
> So that means the API test is wrong (at risk of randomly crashing).

If it's wrong, I'm not sure what's right though... It is consistent that WTF::initializeThreading() has already been called before the test even begins.

> setenv() is *really* tricky! (See bug #194370.)

I did base the WebProcessPool changes here on your patch over there. :)
Comment 45 Michael Catanzaro 2019-02-11 10:56:50 PST
(In reply to Ross Kirsling from comment #44)
> If it's wrong, I'm not sure what's right though... It is consistent that
> WTF::initializeThreading() has already been called before the test even
> begins.

So if you want to test [un]setenv, you know you have to do that before threads are initialized. Right? I'm not sure how you plan to test it.
Comment 46 Ross Kirsling 2019-02-11 11:10:29 PST
(In reply to Michael Catanzaro from comment #45)
> (In reply to Ross Kirsling from comment #44)
> > If it's wrong, I'm not sure what's right though... It is consistent that
> > WTF::initializeThreading() has already been called before the test even
> > begins.
> 
> So if you want to test [un]setenv, you know you have to do that before
> threads are initialized. Right? I'm not sure how you plan to test it.

I don't know either -- perhaps that function ought to just do an immutable string manipulation without editing the env vars itself. I'm happy to create a new bug for redoing EnvironmentUtilities, but I figure that this patch is large enough as it is, hehe.
Comment 47 Ross Kirsling 2019-02-11 14:23:02 PST
Chris on my team pointed out that it's kind of unfortunate that we can't use `if (auto ...` and have to use !! everywhere due to Environment::get's String return type, and that Optional<String> would make life easier.

I originally felt like Optional would be unnecessary wrapping given that a String may have a null impl, but in the end, I think it's better for usability so I'm going to switch.
Comment 48 Ross Kirsling 2019-02-11 17:17:06 PST
Created attachment 361736 [details]
Patch
Comment 49 Build Bot 2019-02-11 17:21:07 PST Comment hidden (obsolete)
Comment 50 Build Bot 2019-02-11 20:14:07 PST Comment hidden (obsolete)
Comment 51 Build Bot 2019-02-11 20:14:09 PST Comment hidden (obsolete)
Comment 52 Ross Kirsling 2019-02-11 20:19:20 PST
Created attachment 361759 [details]
Patch
Comment 53 Build Bot 2019-02-11 20:21:36 PST Comment hidden (obsolete)
Comment 54 Ross Kirsling 2019-02-11 21:29:22 PST
Created attachment 361765 [details]
Patch
Comment 55 Build Bot 2019-02-11 21:32:52 PST Comment hidden (obsolete)
Comment 56 Build Bot 2019-02-11 23:27:43 PST Comment hidden (obsolete)
Comment 57 Build Bot 2019-02-11 23:27:45 PST Comment hidden (obsolete)
Comment 58 Ross Kirsling 2019-02-12 11:27:37 PST
Created attachment 361813 [details]
Patch
Comment 59 Ross Kirsling 2019-02-12 14:22:08 PST
Created attachment 361840 [details]
Patch
Comment 60 Build Bot 2019-02-12 14:25:10 PST Comment hidden (obsolete)
Comment 61 Ross Kirsling 2019-02-12 16:07:45 PST
This should hopefully be ready now.
Comment 62 Michael Catanzaro 2019-02-13 10:57:56 PST
Comment on attachment 361840 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=361840&action=review

Nice!

I bet it will break GTK API tests in debug, but if so, that's just exposing preexisting problems that can be handled separately.

> Source/WTF/wtf/Environment.h:76
> +WTF_EXPORT_PRIVATE void setIfUndefined(const String& variable, const String& value);

How about setIfNotDefined?

> Source/WTF/wtf/posix/FileSystemPOSIX.cpp:429
> -    const char* tmpDir = getenv("TMPDIR");
>  
> +    auto tmpDir = Environment::getRaw("TMPDIR");

Any reason you're changing char* to auto like this? It just makes it harder to read; now we have to look int Environment.h to see it returns a const char*. I usually reserve auto for when there's a cast on the rhs or the type is long.

At least use auto* so we can tell it's a pointer.

> Source/WTF/wtf/win/EnvironmentWin.cpp:55
> +void setIfUndefined(const String& variable, const String& value)
> +{
> +    if (get(variable))

I'd do the ASSERT here too, to ensure it crashes even if the variable is already defined. No problem to have an extra assert since it will get compiled out in release builds.
Comment 63 Ross Kirsling 2019-02-13 11:17:24 PST
(In reply to Michael Catanzaro from comment #62)
> > Source/WTF/wtf/posix/FileSystemPOSIX.cpp:429
> > -    const char* tmpDir = getenv("TMPDIR");
> >  
> > +    auto tmpDir = Environment::getRaw("TMPDIR");
> 
> Any reason you're changing char* to auto like this? It just makes it harder
> to read; now we have to look int Environment.h to see it returns a const
> char*. I usually reserve auto for when there's a cast on the rhs or the type
> is long.
> 
> At least use auto* so we can tell it's a pointer.

I was just following the strategy (from, say, Effective Modern C++) of preferring auto for locals in general, though WK certainly has a mix of both practices and doesn't specify either in its style guide. I totally agree that dropping the * isn't ideal though, and actually the vast majority of these could be explicitly const.
Comment 64 Ross Kirsling 2019-02-13 13:36:54 PST
Created attachment 361933 [details]
Patch
Comment 65 Build Bot 2019-02-13 13:41:31 PST Comment hidden (obsolete)
Comment 66 Ross Kirsling 2019-02-13 13:54:30 PST
(In reply to Ross Kirsling from comment #63)
> I was just following the strategy (from, say, Effective Modern C++) of
> preferring auto for locals in general, though WK certainly has a mix of both
> practices and doesn't specify either in its style guide. I totally agree
> that dropping the * isn't ideal though, and actually the vast majority of
> these could be explicitly const.

Went with `const char*` instead of `const auto*` after all -- I don't feel too strongly either way, but my teammate Steph pointed out that (1) Effective Modern C++ would probably also recommend against const char* where avoidable and (2) unlike `int`, `char` would be hard to replace with something else and have every callsite "just work".
Comment 67 Ross Kirsling 2019-02-13 13:59:43 PST
Created attachment 361937 [details]
Patch
Comment 68 Build Bot 2019-02-13 14:02:32 PST Comment hidden (obsolete)
Comment 69 Michael Catanzaro 2019-02-13 14:08:13 PST
(In reply to Ross Kirsling from comment #66)
> (2) unlike `int`, `char` would be hard to replace with
> something else and have every callsite "just work".

Hm yes, auto is often safer for integer types, that's true.
Comment 70 Ross Kirsling 2019-02-13 14:38:35 PST
Also created bug #194612.
Comment 71 Ross Kirsling 2019-02-13 15:07:17 PST
Created attachment 361942 [details]
Patch
Comment 72 Build Bot 2019-02-13 15:10:39 PST Comment hidden (obsolete)
Comment 73 Ross Kirsling 2019-02-13 17:06:32 PST
WinCairo EWS failure here is a fluke...
Comment 74 Michael Catanzaro 2019-02-14 10:05:53 PST
Comment on attachment 361942 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=361942&action=review

Looks good, r=me.

I still think you're overusing auto (one of the rare cases I will disagree with Scott Meyers ;) but we don't have a style rule about that currently, and this is hardly uncommon in WebKit, and the callsites are otherwise significantly easier to read now anyway, thanks to your convenience functions.

> Tools/WebKitTestRunner/InjectedBundle/gtk/InjectedBundleGtk.cpp:48
> -    if (!g_getenv("WEBKIT_TOP_LEVEL"))
> -        g_setenv("WEBKIT_TOP_LEVEL", TOP_LEVEL_DIR, FALSE);
> +    g_setenv("WEBKIT_TOP_LEVEL", TOP_LEVEL_DIR, FALSE);

Well that doesn't look finished. It should use Environment::set, right? If it asserts, we can try to fix it in a followup (just needs moved to the top of web process main).
Comment 75 Ross Kirsling 2019-02-14 10:17:31 PST
(In reply to Michael Catanzaro from comment #74)
> Comment on attachment 361942 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=361942&action=review
> 
> Looks good, r=me.

Thanks a bunch!

> I still think you're overusing auto (one of the rare cases I will disagree
> with Scott Meyers ;) but we don't have a style rule about that currently,
> and this is hardly uncommon in WebKit, and the callsites are otherwise
> significantly easier to read now anyway, thanks to your convenience
> functions.

One way or another, I too would love to have a clear style rule. :) Perhaps I should send a question to webkit-dev.

> > Tools/WebKitTestRunner/InjectedBundle/gtk/InjectedBundleGtk.cpp:48
> > -    if (!g_getenv("WEBKIT_TOP_LEVEL"))
> > -        g_setenv("WEBKIT_TOP_LEVEL", TOP_LEVEL_DIR, FALSE);
> > +    g_setenv("WEBKIT_TOP_LEVEL", TOP_LEVEL_DIR, FALSE);
> 
> Well that doesn't look finished. It should use Environment::set, right? If
> it asserts, we can try to fix it in a followup (just needs moved to the top
> of web process main).

Basically Mac was asserting from InjectedBundle::platformInitialize so I reverted changes to it for all platforms (after the last patch labelled "All but the ASSERT"), it's just that GTK+ also had a redundant conditional, so I figured I'd still delete it, haha. I can surely make this Environment::set right now, I just didn't want to tank your test bot. :(
Comment 76 Michael Catanzaro 2019-02-14 10:28:22 PST
How about moving it to the top of web process main?
Comment 77 Ross Kirsling 2019-02-14 10:53:30 PST
Created attachment 362038 [details]
Patch for landing
Comment 78 Ross Kirsling 2019-02-14 10:55:59 PST
Created attachment 362040 [details]
Patch for landing
Comment 79 Michael Catanzaro 2019-02-14 11:22:55 PST
Comment on attachment 362040 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=362040&action=review

> Source/WebKit/WebProcess/gtk/WebProcessMainGtk.cpp:49
> +        Environment::setIfNotDefined("WEBKIT_TOP_LEVEL", TOP_LEVEL_DIR);

Ah whoops, my bad. That's from WebKitTestRunner's InjectedBundle... we're moving this from Tools/ to Source/ :O

Best just leave this for a follow-up bug.
Comment 80 Ross Kirsling 2019-02-14 11:23:56 PST
(In reply to Michael Catanzaro from comment #79)
> Comment on attachment 362040 [details]
> Patch for landing
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=362040&action=review
> 
> > Source/WebKit/WebProcess/gtk/WebProcessMainGtk.cpp:49
> > +        Environment::setIfNotDefined("WEBKIT_TOP_LEVEL", TOP_LEVEL_DIR);
> 
> Ah whoops, my bad. That's from WebKitTestRunner's InjectedBundle... we're
> moving this from Tools/ to Source/ :O
> 
> Best just leave this for a follow-up bug.

Oops! Alrighty then. 😅
Comment 81 Ross Kirsling 2019-02-14 11:25:12 PST
Created attachment 362041 [details]
Patch for landing
Comment 82 WebKit Commit Bot 2019-02-14 12:03:35 PST
Comment on attachment 362041 [details]
Patch for landing

Clearing flags on attachment: 362041

Committed r241559: <https://trac.webkit.org/changeset/241559>
Comment 83 WebKit Commit Bot 2019-02-14 12:03:37 PST
All reviewed patches have been landed.  Closing bug.
Comment 84 Radar WebKit Bug Importer 2019-02-14 12:04:39 PST
<rdar://problem/48082754>
Comment 85 Michael Catanzaro 2019-02-14 13:44:58 PST
(In reply to Fujii Hironori from comment #3)
> Accounting to the glibc document, it is safe to call getenv in multiple
> threads, but not safe with other libc.

Forgot about this (from way up at the top of the bug). Fujii is right, even getenv() isn't necessarily safe. But removing use of getenv() would be very difficult, probably not practical.
Comment 86 Fujii Hironori 2019-02-14 17:27:02 PST
(In reply to Michael Catanzaro from comment #85)
> Forgot about this (from way up at the top of the bug). Fujii is right, even
> getenv() isn't necessarily safe. But removing use of getenv() would be very
> difficult, probably not practical.

If it will become a real problem, we can use getenv_s.
Comment 87 Ryan Haddad 2019-02-15 10:02:24 PST
(In reply to WebKit Commit Bot from comment #82)
> Committed r241559: <https://trac.webkit.org/changeset/241559>

I'm seeing crashes when running layout tests with GuardMalloc on Mojave after this change:

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.WebKit              	0x0000000101f4f26d WebKit::EnvironmentUtilities::stripValuesEndingWithString(char const*, char const*) + 196
1   com.apple.WebKit              	0x0000000101f50ce2 NetworkServiceInitializer + 38
2   com.apple.WebKit              	0x00000001020af8a6 invocation function for block in WebKit::XPCEventHandler(NSObject<OS_xpc_object>*, WebKit::AuxiliaryProcessType) + 293
3   libxpc.dylib                  	0x00007fff616cc044 _xpc_connection_call_event_handler + 56
4   libxpc.dylib                  	0x00007fff616c9fda _xpc_connection_mach_event + 933
5   libdispatch.dylib             	0x00007fff6144a6dd _dispatch_client_callout4 + 9
6   libdispatch.dylib             	0x00007fff6145f0d6 _dispatch_mach_msg_invoke + 436
7   libdispatch.dylib             	0x00007fff61450792 _dispatch_lane_serial_drain + 268
8   libdispatch.dylib             	0x00007fff6145fc19 _dispatch_mach_invoke + 481
9   libdispatch.dylib             	0x00007fff6145554b _dispatch_main_queue_callback_4CF + 813
10  com.apple.CoreFoundation      	0x00007fff34a3d227 __CFRUNLOOP_IS_SERVICING_THE_MAIN_DISPATCH_QUEUE__ + 9
11  com.apple.CoreFoundation      	0x00007fff34a3c951 __CFRunLoopRun + 2289
12  com.apple.CoreFoundation      	0x00007fff34a3be0e CFRunLoopRunSpecific + 455
13  com.apple.Foundation          	0x00007fff36d57a9f -[NSRunLoop(NSRunLoop) runMode:beforeDate:] + 280
14  com.apple.Foundation          	0x00007fff36d57974 -[NSRunLoop(NSRunLoop) run] + 76
15  libxpc.dylib                  	0x00007fff616d01d7 _xpc_objc_main + 552
16  libxpc.dylib                  	0x00007fff616cfcd9 xpc_main + 433
17  com.apple.WebKit              	0x00000001020afa0d WebKit::XPCServiceMain(int, char const**) + 105
18  libdyld.dylib                 	0x00007fff614973d5 start + 1
Comment 88 Ryan Haddad 2019-02-15 10:14:40 PST
(In reply to Ryan Haddad from comment #87)
> (In reply to WebKit Commit Bot from comment #82)
> > Committed r241559: <https://trac.webkit.org/changeset/241559>
> 
> I'm seeing crashes when running layout tests with GuardMalloc on Mojave
> after this change:

This seems to be 100% reproducible and blocks testing with GuardMalloc. I'm going to roll this out.
Comment 89 WebKit Commit Bot 2019-02-15 10:20:14 PST
Re-opened since this is blocked by bug 194710
Comment 90 Michael Catanzaro 2019-02-15 10:29:58 PST
(In reply to Ryan Haddad from comment #87)
> (In reply to WebKit Commit Bot from comment #82)
> > Committed r241559: <https://trac.webkit.org/changeset/241559>
> 
> I'm seeing crashes when running layout tests with GuardMalloc on Mojave
> after this change:
> 
> Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
> 0   com.apple.WebKit              	0x0000000101f4f26d
> WebKit::EnvironmentUtilities::stripValuesEndingWithString(char const*, char
> const*) + 196

Strange, it looks like the commit left that function unchanged (broken, for follow-up in bug #194612) specifically to avoid surfacing crashes on the debug bots. Since we didn't change it, it's not clear why it's only crashing now. Anyway, guess we need to deal with it now.
Comment 91 Ross Kirsling 2019-02-15 13:48:27 PST
What is GuardMalloc and how would I test with it on a Mojave machine?

Not sure why getRaw would cause a problem, but then there was also a heisenbug in WKTR TestController (somehow related to CFString conversion) that went away by refactoring the callsites. One way or another, if rewriting EnvironmentUtilities::stripValuesEndingWithString is next on my list anyway, I suppose it makes sense to just drop it from this patch entirely.
Comment 92 Ross Kirsling 2019-02-15 13:50:11 PST
Created attachment 362153 [details]
Patch for landing
Comment 93 Ryan Haddad 2019-02-15 13:54:06 PST
(In reply to Ross Kirsling from comment #91)
> What is GuardMalloc and how would I test with it on a Mojave machine?
It is a testing mode to find memory access bugs. I encountered the crashes with a release build and the following layout test invocation:
run-webkit-tests --guard-malloc
Comment 94 Ross Kirsling 2019-02-15 14:08:46 PST
Created attachment 362156 [details]
Patch for landing
Comment 95 Ross Kirsling 2019-02-15 14:10:43 PST
(In reply to Ryan Haddad from comment #93)
> (In reply to Ross Kirsling from comment #91)
> > What is GuardMalloc and how would I test with it on a Mojave machine?
> It is a testing mode to find memory access bugs. I encountered the crashes
> with a release build and the following layout test invocation:
> run-webkit-tests --guard-malloc

Thanks! Will be sure to make use of that in the follow-up.
Comment 96 WebKit Commit Bot 2019-02-15 14:47:33 PST
Comment on attachment 362156 [details]
Patch for landing

Clearing flags on attachment: 362156

Committed r241620: <https://trac.webkit.org/changeset/241620>
Comment 97 WebKit Commit Bot 2019-02-15 14:47:36 PST
All reviewed patches have been landed.  Closing bug.
Comment 98 David Kilzer (:ddkilzer) 2019-02-17 15:54:36 PST
Comment on attachment 362156 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=362156&action=review

> Source/WTF/wtf/Environment.h:39
> +inline const char* getRaw(const String& variable)
> +{
> +    auto string = get(variable);
> +    return string ? string->utf8().data() : nullptr;
> +}

This method is returning freed memory which is causing crashes on GuardMalloc and ASan bots.

The const char* pointer being returned is owned by the WTF::String contained in the WTF::Optional<WTF::String>.  Once `string` goes out of scope, the memory is freed.

This function should probably return a std::unique_ptr<> to a const char* to manage its lifetime, or (better yet?) just inline getenv(variable.utf8().data()) for POSIX systems, which is what the code it's replacing did.
Comment 99 David Kilzer (:ddkilzer) 2019-02-17 16:12:52 PST
(In reply to WebKit Commit Bot from comment #96)
> Comment on attachment 362156 [details]
> Patch for landing
> 
> Clearing flags on attachment: 362156
> 
> Committed r241620: <https://trac.webkit.org/changeset/241620>

Reverted:

Committed r241654: <https://trac.webkit.org/changeset/241654>

"Causes use-after-free crashes running layout tests with ASan and GuardMalloc."
(Requested by ddkilzer on #webkit.)
Comment 100 Michael Catanzaro 2019-02-17 16:34:36 PST
(In reply to David Kilzer (:ddkilzer) from comment #98)
> string->utf8().data()

Such a common mistake, and no clear way to make the CString interface safer. :/
Comment 101 Ross Kirsling 2019-02-17 16:53:21 PST
(In reply to Michael Catanzaro from comment #100)
> (In reply to David Kilzer (:ddkilzer) from comment #98)
> > string->utf8().data()
> 
> Such a common mistake, and no clear way to make the CString interface safer.
> :/

Ugh, right... "necessary but slightly less convenient" was the intention with getRaw, but botching lifetimes certainly was not.

I suppose we could just make getRaw a literal passthrough to std::getenv, as CurlContext's EnvironmentVariableReader (part of the inspiration for this patch) was doing:
https://github.com/WebKit/webkit/blob/82e5788b7127fde8aa30bbf72c11b648cf5cfe67/Source/WebCore/platform/network/curl/CurlContext.cpp#L52

This won't handle multibyte text on Windows correctly, but if the function is understood to be use-at-your-own-risk...?
Comment 102 Darin Adler 2019-02-18 06:45:00 PST
I’m not thrilled with this. I am not sure we should be adding this platform layer.

Do we need our own interface to environment variables? Is this so heavily used throughout WebKit that this will truly be helpful?

This same kind of issue came up before with international text. Someone was building a platform layer that we used instead of calling ICU directly, and eventually we chose not to add one because we didn’t want to layer a WebKit abstraction on top of everything from ICU. We need to be careful that we don’t create our own WebKit versions of things unnecessarily.

I see some comments early on about what problems we might be solving, but I am not completely clear on what they are. I would like to understand explicitly what problems specifically we are trying to solve here. We should attempt to create the lightest possible layer to solve them.

It's very easy to introduce problems. To pick one example, conversion of strings from 8-bit characters to and from WTF::String can easily cause problems if we don’t know what the character sets are (are they UTF-8, Latin-1, guaranteed to be ASCII, etc.). I don’t know that all environment variables are consistent in that respect. And it's easy to make something that's poor performance.
Comment 103 Darin Adler 2019-02-18 06:46:29 PST
I would suggest that ideally WebKit should be able to work on platforms that don’t have environment variables at all. I don’t see why we’d want to depend on them.

Maybe we can get rid of the getenv/setenv rather than making them easier to program with? Or at least put them inside platform-specific code rather than core WebKit code.
Comment 104 Ross Kirsling 2019-02-18 17:45:35 PST
Although the original idea here is not my own, the benefits I see are the following:

1. A String-based interface -- in particular, functions like hasValue and getInt avoid the need for strcmp and sscanf in common operations.

2. ASSERTs to improve thread safety, coarse-grained as they may be -- we can't prevent getenv from being called all over the place, but we can actively seek to restrict where [un]setenv gets called.

3. Consistency improvements due to a platform-independent interface:

   - Do away with g_*env (the usage of which is currently inconsistent and debatably superfluous).

   - Use GetEnvironmentVariableW for proper multibyte charset support on Windows (which is disgustingly longwinded and deserves its own helper if we're not just going to cut corners with std::getenv).

   - Generally-speaking, avoid the worry over whether charsets are being handled correctly at every single getenv callsite (based on the hopefully-not-mistaken understanding that String::fromUTF8(getenv(...)) is appropriate for POSIX across the board).

Other folks may have further points to make (or corrections to the above).
Comment 105 Ross Kirsling 2019-02-19 10:59:51 PST
Created attachment 362399 [details]
Patch
Comment 106 Build Bot 2019-02-19 11:01:40 PST
Attachment 362399 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/DataLog.cpp:29:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WTF/wtf/posix/EnvironmentPOSIX.cpp:27:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WTF/wtf/win/EnvironmentWin.cpp:27:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 3 in 84 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 107 Ross Kirsling 2019-02-19 11:22:26 PST
For now, here is a patch that addresses the UAF (by having getRaw be a literal std::getenv passthrough).

We still need to decide whether there's sufficient justification for this API, but here's one more point I thought of for the list:

4. There are 71 unique env var literals touched in this patch, and that is certainly not the full list of env vars handled by WK -- minimizing them is surely a good idea but would be a massive undertaking.
Comment 108 Michael Catanzaro 2019-02-19 14:28:21 PST
TBH I was just thinking the other day that it would be nice to have a WebKit-level wrapper around ICU, simply because the ICU API is hard to use correctly and a recent bug could have been avoided with a sane wrapper.

Weaning WebKit away from using environment variables sadly does not seem very practical, but fortunately I think almost all of them are mainly there just for debugging.
Comment 109 Darin Adler 2019-02-19 17:17:59 PST
(In reply to Michael Catanzaro from comment #108)
> TBH I was just thinking the other day that it would be nice to have a
> WebKit-level wrapper around ICU, simply because the ICU API is hard to use
> correctly and a recent bug could have been avoided with a sane wrapper.

I’d like to hear more about that, but maybe this bug is not the place to discuss this. We exclusively use ICU’s C API, likely because of concerns about ABI stability; maybe the C++ one is easier to use?

I’m concerned about WebKit having special solutions to every problem, rather than relying on standard solutions that are already widely available and understood. Our platform layers and utility libraries tend to grow and personally, I like to do what I can to shrink them.
Comment 110 Ross Kirsling 2019-02-19 18:41:45 PST
Given the described benefits and the actual state of the patch itself, are there further conditions that would need to be met in order to be considered justifiable?

Note that the per-platform implementations are in fact maximally thin (given a String-based interface) and that the helper functions simplify code in many locations throughout the codebase.

The abstract concern over not using the POSIX API directly is the singular negative I'm seeing, but note that after this patch, the only bare calls that remain are in places that can't use the WTF API without non-trivial modification, so potentially this gives us a way to identify areas for improvement with respect to env var management (most notably, in the places where we'd be hitting the ASSERTs).
Comment 111 Keith Rollin 2019-02-21 00:24:47 PST
I agree with Darin's concerns. It's not clear to me that the layer is worth the benefit, it's not clear to me what the encoding guarantees are, and it would be nice to get rid of the use of environment variables anyway (having WebKit open to input via environment variables seems like a possible security issue).

But that aside, I mostly wanted to ask about the use of 'const String&' in the interface. In all of the places I spot-checked, the environment variable names are being passed by the caller as a 'const char*' or a string literal. As the Environment interface stands, these variable names will be used to create String objects, which then internally are essentially converted back to a 'const char*'. There doesn't seem to be any benefit to having the interface take String references, while at the same time it eats up memory and CPU cycles. Perhaps a StringView would be more appropriate, if there's some reason for not just taking a 'const char*'?
Comment 112 Ross Kirsling 2019-02-21 09:58:17 PST
(In reply to Keith Rollin from comment #111)
> I agree with Darin's concerns. It's not clear to me that the layer is worth
> the benefit, it's not clear to me what the encoding guarantees are, and it
> would be nice to get rid of the use of environment variables anyway (having
> WebKit open to input via environment variables seems like a possible
> security issue).
> 
> But that aside, I mostly wanted to ask about the use of 'const String&' in
> the interface. In all of the places I spot-checked, the environment variable
> names are being passed by the caller as a 'const char*' or a string literal.
> As the Environment interface stands, these variable names will be used to
> create String objects, which then internally are essentially converted back
> to a 'const char*'. There doesn't seem to be any benefit to having the
> interface take String references, while at the same time it eats up memory
> and CPU cycles. Perhaps a StringView would be more appropriate, if there's
> some reason for not just taking a 'const char*'?

You're absolutely right, there's no excuse for it not at least being a StringView! The only reason it's not `const char*` is it because it seemed perhaps inappropriate to *require* a raw string (at least for the values if not for the variable names), although this isn't necessarily a strong argument since, as you mentioned, it's not a problem for any existing callsites.

---

Of the points I've made, I can understand if (1) seems a mere nice-to-have and if, in spite of (4), we don't want to encourage even broader use of environment variables by the introduction of a new API. I would like to think that (3) is a reasonably strong argument, but if the non-Unix code in this patch doesn't actually modify any environment variables, then perhaps the worry over having a platform-independent [un]setenv is less compelling.

One way or another, (2) deserves recognition for the problem that it is, whether or not we like the proposed assertion. I suppose you could argue that seeking out existing unsafe [un]setenv calls and preventing future ones is not in itself sufficient justification for a new API, but then it seems we should be discussing alternatives.
Comment 113 Stephan Szabo 2019-02-21 10:52:50 PST
One other issue with the current situation is that it seems that getenv()'s Windows behavior is such that using it in a UNICODE application may be problematic and have performance implications due to having two environments to maintain.

That may not be a huge issue if the only uses are for flags, but if any of the environment variable uses are for paths, then this is potentially a problem as it's unclear what happens to unicode paths in the environment that aren't representable in the current mbcs code page.
Comment 114 Ross Kirsling 2019-02-21 11:25:06 PST
(In reply to Stephan Szabo from comment #113)
> One other issue with the current situation is that it seems that getenv()'s
> Windows behavior is such that using it in a UNICODE application may be
> problematic and have performance implications due to having two environments
> to maintain.
> 
> That may not be a huge issue if the only uses are for flags, but if any of
> the environment variable uses are for paths, then this is potentially a
> problem as it's unclear what happens to unicode paths in the environment
> that aren't representable in the current mbcs code page.

Indeed -- so then at a minimum, we'll need the fixes for CURL_LOG_FILE, CURL_COOKIE_JAR_PATH, and CURL_CA_BUNDLE_PATH as well as the fixes for the three copies of LoggingWin.cpp, and these will all be repeating the same half-dozen lines.

So then we certainly need a Windows-oriented "use me instead of getenv" replacement, but if this isn't a proper platform abstraction, then any cross-platform code is almost certain to forget to use it (or it'll just happen implicitly, e.g. via JSC's Options.cpp).

So I think that bolsters argument (3) on the basis of getenv alone.
Comment 115 Darin Adler 2019-02-21 17:03:49 PST
Something we have done for Windows before is to make Windows covers that rationalize Windows behavior and make it work like Linux in WebKit source files, using macros and the like. This was a really successful strategy in the past.

Maybe we can do that for getenv, using a macro to map it to a version of getenv that does exactly what we want on Windows, something that we put in WTF.

The callers can still just pretend they are calling the "real" getenv.
Comment 116 Michael Catanzaro 2019-02-21 17:15:14 PST
(In reply to Darin Adler from comment #115)
> Something we have done for Windows before is to make Windows covers that
> rationalize Windows behavior and make it work like Linux in WebKit source
> files, using macros and the like. This was a really successful strategy in
> the past.
> 
> Maybe we can do that for getenv, using a macro to map it to a version of
> getenv that does exactly what we want on Windows, something that we put in
> WTF.
> 
> The callers can still just pretend they are calling the "real" getenv.

I think at this point we should step back a bit and consider what problem we're trying to solve with a macro-based solution like that, which is potentially quite confusing. We've also had trouble in the past caused by sneakily redefining standard things (e.g. our custom std::optional).

Avoiding an unnecessary abstraction layer is a laudable goal. But if it's actually useful to have the abstractions -- and in this case, it sounds so to me, from the arguments above -- then that's fine.
Comment 117 Darin Adler 2019-02-21 18:56:22 PST
OK I understand what you are saying, but I don’t agree.
Comment 118 Don Olmstead 2019-02-22 11:10:42 PST
It seems like we're all at an impasse over this patch.

The original idea with this bug was that we weren't being consistent in our usage of environment variables. There were many cases of platform specific calls, g_get_env, GetEnvironmentVariableW. On top of that there were things like conversions to different types. There was some code done in cURL that had some of those conversions. Michael also mentioned the potential safety issues around threading and using environment variables. So this seemed like a neighborly thing to do from our end hence the patch.

Darin has a great point that environment variables are a horrible way to configure things. I don't think anyone can disagree with that. However it'll be some amount of work to remove those environment variables throughout the codebase.

So taking a step back the problem we want to solve is runtime configuration. With environment variables they're primarily used during development, though Michael mentioned that in some cases GTK users twiddle some to help debug problems in the wild. I'm wondering if its a better approach to have a unified concept of settings for running WebKit code. When it comes to environment variables that can be a backend for getting those values. That path could only be enabled for DEVELOPER_MODE so we can still use these things for testing and during development. There could be other paths for setting these values. If Apple plans on releasing iTunes as a Universal Windows Platform application environment variables are strictly disallowed so maybe theres a Windows Registry backend in that case. GLib has the concept of GSettings so maybe thats a GLib backend. Anyways that could be a longer term plan to get these things under control.

At this point we can land this patch as is or just keep things as they are. The benefit of landing is additional safety and having a common interface across platforms. It also gives us a common API to grep for if we want to try for some unified settings in the future. I don't personally think this patch means that we approve of using environment variables for configuration though I can see that being an assumption considering we're exposing an API in this patch. If we think of this as a stepping stone to something better perhaps its more palatable.

Anyways we'd like to know if this can land or if we just need to back off this patch. Thanks for all the constructive criticism on this.