1435445 - Make talos/scripts/MozillaFileLogger.js not use enablePrivileges

Status: RESOLVED FIXED

(Testing :: Talos, enhancement, P3)

Description

[Andrew McCreight [:mccr8]]

This file implements a logger that can be used from content, and uses enablePrivileges. However, I think it can't be e10s compatible, because it creates a file in the content process, unless sandboxing is disabled for Talos. If that's really the case, then maybe I could just remove it entirely?

Comment 1

[Andrew McCreight [:mccr8]]

Joel, does comment 0 sound right to you?

Comment 2

[Joel Maher ( :jmaher ) (UTC -8)]

talos runs in e10s mode on all platforms, I am not aware of sandboxing being disabled.  The logger is used to log from tests and addons- I am happy to see this changed to remove older APIs or support newer methods- Luckily with this I do not think there are any edge cases- just make sure the talos tests run successfully.

Comment 3

[Andrew McCreight [:mccr8]]

I think it is easy enough to make it work, so I'll leave it alone. I know there's some logging I use regularly that doesn't work unless you disable sandboxing.

MozillaFileLogger.js calls enablePrivilege at the top level, and some code that getInfo.html runs (in a different file, it should be noted) seems to implicitly depend on it. I'm not sure what yet.

Comment 4

[Alex Gaynor [:Alex_Gaynor]]

The "talos.logfile" pref, which is where this logs to, is always set to "browser_output.txt" as far as I can tell. The only directory that we have write access to on macOS is the content temp directory (and this is going away soon!), so unless the pwd (or wherever "browser_output.txt" is relative to) is the content temp directory, there's already no way this is working with the macOS sandbox today.

Comment 5

[Joel Maher ( :jmaher ) (UTC -8)]

we need to think of android as well; technically android has an old version of pageloader, but we will be running more tests on android this year

Comment 6

[Andrew McCreight [:mccr8]]

I'm not looking at this right now. Hopefully I'll get around to it at some point if nobody else does....

Comment 7

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Hey Andrew, is there still a need for this? Could you help us to determine the priority?

Comment 8

[Andrew McCreight [:mccr8]]

(In reply to Dave Hunt [:davehunt] [he/him] ⌚BST from comment #7)

Hey Andrew, is there still a need for this? Could you help us to determine the priority?

Yes, this is still something we want to do. Talos is the only user of enablePrivileges, which would be nice to remove from XPConnect. P3 sounds about right, though.

Comment 9

[Masatoshi Kimura [:emk]]

I think we can just remove content MozillaFileLogger.js because it serves no useful purpose.
According to this try run,
https://treeherder.mozilla.org/#/jobs?repo=try&revision=051b824128ccccff45837408151a9be3c31c19eb
MozFileLogger.init always fails (at least in the content scope.)
(Note that try is "green" due to bug 1602775.)

Comment 10

[Masatoshi Kimura [:emk]]

(In reply to Joel Maher ( :jmaher ) (UTC-4) from comment #5)

we need to think of android as well; technically android has an old version
of pageloader, but we will be running more tests on android this year

Is Talos still running on Android? I couldn't find Talos jobs on Treeherder.

Comment 11

[Masatoshi Kimura [:emk]]

The error is getCharPref failure due to absense of the talos.logfile pref. Currently we don't even set the pref correctly and nobody cares about that.

Comment 12

[Joel Maher ( :jmaher ) (UTC -8)]

talos is not on Android, so that removes a lot of needs for writing to files, I believe it is just stdout for getting data. There could be a test that depends on MFL, but a quick search doesn't turn up anything.

I haven't worked on talos for over a year, I think :davehunt would know the best person to work with to resolve this bug.

Comment 13

[Masatoshi Kimura [:emk]]

Attachment: Bug 1435445 - Remove talos/scripts/MozillaFileLogger.js. r=rwood

Depends on D56769

Comment 14

[Stephen Donner [:stephend] Not actively reading bugmail]

Thanks for the patch, Masatoshi-san! Do you have a try server push you can link to here, for these changes? I poked around and saw https://treeherder.mozilla.org/#/jobs?repo=try&tier=1%2C2%2C3&revision=ff43bad9335e5e1e53b4896de723d0e70a55583c but that has several user-cancels, so just wondering.

Comment 15

[Masatoshi Kimura [:emk]]

Here is the try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9a8905d066defe8757e13d8fc935c73bf4812e49

This also contains a fix for bug 1602775 because otherwise failures won't turn the jobs to orange.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/13333c1a4e66
Remove talos/scripts/MozillaFileLogger.js. r=rwood,perftest-reviewers

Comment 17

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/13333c1a4e66

Comment 18

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/13333c1a4e66