1407059 - GECKO_DISPLAY_REFLOW_RULES_FILE doesn't work when security.sandbox.content.level is 3 (or more?)

Status: RESOLVED FIXED

(Core :: Layout, defect, P3)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The reflow debugging tool GECKO_DISPLAY_REFLOW_RULES_FILE doesn't work when the preference security.sandbox.content.level is 3 (which is now the default on mozilla-central).  This is because it relies on reading the file given in the environment variable, in the content process.

Using --setpref security.sandbox.content.level=2 works as a workaround.

I only actually ever use this to point to a file whose contents are "* 1"; I'm not sure if other layout developers use anything fancier.  It's possible we could restructure the debugging to not use a file, but just have the rules in the environment variable.

cc:ing a few other people who might use the pref to see what they think about that.

Comment 1

[Daniel Holbert [:dholbert]]

(In reply to David Baron :dbaron: ⌚️UTC-7 from comment #0)
> I only actually ever use this to point to a file whose contents are "* 1";

(Same with me -- though I rarely use this feature (though I perhaps should use it more often)).

> It's possible we could restructure the debugging to not use a file,
> but just have the rules in the environment variable.

That seems like a good idea to me.  (That might require simplifying the syntax a bit -- to be honest I don't recall the current syntax beyond "* 1" -- but that's perhaps worth doing anyway.)

Comment 2

[Mats Palmgren (inactive)]

I use it very rarely, and haven't used any fancy rules IIRC.
Removing _FILE from the name and making it contain the rules directly
sounds good to me.

A simpler alternative would be to just exit with an error message if
GECKO_DISPLAY_REFLOW_RULES_FILE is set but opening the file failed,
with a suggestion that the above --setpref might be needed?

Comment 3

[Liz Henry (:lizzard) (relman/hg->git project)]

Too late for fixing in 59. 
Setting as fix-optional to exclude it from triage, since it has a P3 priority.

Comment 4

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

I'm still learning about reflow, so I'm interested in using GECKO_DISPLAY_REFLOW_RULES_FILE to better understand what's happening in bug 1452277.
I hit the sandbox issue when I tried to rr-record a session.

As a quick&dirty local fix, before trying to open the file, I handle some special names that don't actually access any files, e.g.:
- GECKO_DISPLAY_REFLOW_RULES_FILE="" -> Logs everything like "* 1".
- GECKO_DISPLAY_REFLOW_RULES_FILE="c" -> Logs everything, but only from the content process; nice to reduce output.

Anyone interested?
Thoughts/suggestions?

(In reply to Mats Palmgren (:mats) from comment #2)
> Removing _FILE from the name and making it contain the rules directly
> sounds good to me.
I'd prefer not to have to parse the envvar string (at least as a first quick useful patch here, it could still be done later on if someone needs it) -- It doesn't look trivial to implement, because the parser works with a FILE*.

> A simpler alternative would be to just exit with an error message if
> GECKO_DISPLAY_REFLOW_RULES_FILE is set but opening the file failed,
> with a suggestion that the above --setpref might be needed?
It already MOZ_CRASHes; we could add the --setpref suggestion to the message there.

Comment 5

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: Bug 1407059 - GECKO_DISPLAY_REFLOW_PROCESSES=a/p/c enables reflow logging in chosen processes -

GECKO_DISPLAY_REFLOW_RULES_FILE doesn't work when the sandbox prevents access
to the rules file.

As a workaround, and because in most cases the rules are only "* 1" (log
everything), this new env-var GECKO_DISPLAY_REFLOW_PROCESSES=a/p/e/c enables
all reflow logging without needing a rules file.

It also adds the choice of which processes have logging enabled, based on the
giving letter:
- a: All processes,
- p: Parent process (in non-e10s mode),
- e: e10s parent process,
- c: e10s content processes.

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

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

This looks pretty reasonable to me, although I don't think it's worth distinguishing between XRE_IsParentProcess() and XRE_IsE10sParentProcess() (the latter being a subset of the former).  Maybe just use a/p/c?

Comment 7

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

True, thanks for the suggestion, will do.

Comment 8

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8989329 [details]
Bug 1407059 - GECKO_DISPLAY_REFLOW_PROCESSES=a/p/c enables reflow logging in chosen processes -

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

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8989329 [details]
Bug 1407059 - GECKO_DISPLAY_REFLOW_PROCESSES=a/p/c enables reflow logging in chosen processes -

https://reviewboard.mozilla.org/r/254396/#review261214

::: layout/doc/frame_reflow_debug.html:48
(Diff revision 2)
> +<ul>
> +<li><code>set GECKO_DISPLAY_REFLOW_PROCESSES=a</code> to log from all processes
> +<li><code>set GECKO_DISPLAY_REFLOW_PROCESSES=p</code> to log from the parent process
> +<li><code>set GECKO_DISPLAY_REFLOW_PROCESSES=c</code> to log from e10s content processes
> +</ul>
> +<li>OR create a rules file:

maybe mention having to mess with the sandbox pref?

Comment 10

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8989329 [details]
Bug 1407059 - GECKO_DISPLAY_REFLOW_PROCESSES=a/p/c enables reflow logging in chosen processes -

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

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4f695620856d
GECKO_DISPLAY_REFLOW_PROCESSES=a/p/c enables reflow logging in chosen processes - r=dbaron

Comment 12

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/4f695620856d