Bug 188002

Created attachment 345757 [details]
Patch

Comment on attachment 345757 [details]
Patch

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

Reviewing informally... I have a comment about this, please read below.

> Tools/flatpak/flatpakutils.py:683
> +                if envvar.split("_")[0] in ("GST", "GTK", "G", "WEBKIT", "WEBKIT2") or \

I would also add variables with the WPE_ prefix. At least WPEBackend checks
WPE_BACKEND_LIBRARY in developer mode builds, and WPEBackend-rdk also picks
some variables from the environment like WPE_INIT_VIEW_{WIDTH,HEIGHT} and
WPE_BCMRPI_{TOUCH,CURSOR}.

Created attachment 345759 [details]
Patch

Thanks for the review Adrian, WPE_ prefixed variables are passed through as well now

I think we should just pass through the entire environment. Look at https://trac.webkit.org/wiki/EnvironmentVariables. We also need at least GIGACAGE_ENABLED, JavaScriptCoreUseJIT, and Malloc.

What still  uses WEBKIT2? Any such environment variable should probably be renamed.

(In reply to Michael Catanzaro from comment #4)
> I think we should just pass through the entire environment. Look at
> https://trac.webkit.org/wiki/EnvironmentVariables. We also need at least
> GIGACAGE_ENABLED, JavaScriptCoreUseJIT, and Malloc.
> 
> What still  uses WEBKIT2? Any such environment variable should probably be
> renamed.

There's this one for pausing the Web process at launch time:

  % rg 'getenv.*WEBKIT2' Source/
  Source/WebKit/WebProcess/gtk/WebProcessMainGtk.cpp
  49:        if (g_getenv("WEBKIT2_PAUSE_WEB_PROCESS_ON_LAUNCH"))

  Source/WebKit/WebProcess/wpe/WebProcessMainWPE.cpp
  47:        if (g_getenv("WEBKIT2_PAUSE_WEB_PROCESS_ON_LAUNCH"))
  %

(In reply to Michael Catanzaro from comment #4)
> I think we should just pass through the entire environment. Look at
> https://trac.webkit.org/wiki/EnvironmentVariables. We also need at least
> GIGACAGE_ENABLED, JavaScriptCoreUseJIT, and Malloc.

My gut feeling is that passing the entire environment would not be a good idea, the sandbox sets up some environment variables of its own, and generally having uncontrolled environment pollution like that sounds a bit risky. I would prefer to just special case the WebKit environment variables as needed, even if there are a lot of special cases.

> 
> What still  uses WEBKIT2? Any such environment variable should probably be
> renamed.

WEBKIT2_PAUSE_WEB_PROCESS_ON_LAUNCH is the only one I can find.

(In reply to Charlie Turner from comment #6)
> My gut feeling is that passing the entire environment would not be a good
> idea, the sandbox sets up some environment variables of its own, and
> generally having uncontrolled environment pollution like that sounds a bit
> risky. I would prefer to just special case the WebKit environment variables
> as needed, even if there are a lot of special cases.

OK

(In reply to Charlie Turner from comment #6)
> WEBKIT2_PAUSE_WEB_PROCESS_ON_LAUNCH is the only one I can find.

We can consider in another bug whether it's better to rename, or if the cost of breaking old tutorials is too high.

Created attachment 345761 [details]
Patch

FWIW, I agree with Charlie that it's better to pass a controlled
and well-known set of environment variables. Otherwise we may run
into the kind of weird situations which containers and sandboxing
are supposed to fix.

As a matter of fact, I would go as far as reporting which environment
variables are being copied inside the sandbox before entering the
Flatpak environment. That would help replicate the running conditions
reported e.g. by other developers or build bots, for debugging. It can
also help developers notice that they are unintentionally setting
variables.

The commit-queue encountered the following flaky tests while processing attachment 345761 [details]:

fast/repaint/canvas-object-fit.html bug 188004 (author: simon.fraser@apple.com)
The commit-queue is continuing to process your patch.

Comment on attachment 345761 [details]
Patch

Clearing flags on attachment: 345761

Committed r234199: <https://trac.webkit.org/changeset/234199>

All reviewed patches have been landed.  Closing bug.

<rdar://problem/42587131>