1350438 - Should verify that PathUtils.tempDir caching happens after the content process sandbox environment is established and the temp dir doesn't change anymore

Status: RESOLVED INCOMPLETE

(Core :: XPCOM, enhancement)

Description

[Andrew McCreight [:mccr8]]

OSFileConstantsService gets the value of TmpD from the directory service. However, the value of TmpD in a sandboxed content process does not stay the same.

Initially, TmpD has the same value as the parent process, but eventually ContentProcess::Init() calls SetUpSandboxEnvironment() and changes it to a special content process temp directory. This happens fairly early in startup, but not before we can start running JS. If, say, telemetry eagerly loads osfile.jsm then we end up with a stale value for OS.Constants.Path.tmpDir. (See bug 1349389 for more context.)

My patch in bug 1349389 keeps us from loading osfile.jsm until later, but that feels like a shaky invariant. At a minimum we should assert that OSFileConstantsService is started after SetUpSandboxEnvironment(). A slicker approach would be to somehow update the value of OS.Constants.Path.tmpDir but I don't know how much of a pain that would be.

Comment 1

[David Teller [:Yoric] - still alive but not very active]

> A slicker approach would be to somehow update the value of OS.Constants.Path.tmpDir but I don't know how much of a pain that would be.

This would be ideal, but difficult due to threads: OS.Constants can be accessed by any thread, while the directory service can only be accessed from the main thread, so we need to pre-read the values.

Comment 2

[:Gijs (away/busy until Mar 10)]

(In reply to David Teller [:Yoric] (please use "needinfo") from comment #1)
> > A slicker approach would be to somehow update the value of OS.Constants.Path.tmpDir but I don't know how much of a pain that would be.
> 
> This would be ideal, but difficult due to threads: OS.Constants can be
> accessed by any thread, while the directory service can only be accessed
> from the main thread, so we need to pre-read the values.

The "feature" of OS File jsms loading automatically (greedily) fetching various paths out of the directory service is problematic, IMO. It bit me in bug 1344759, see bug 1348103 (where it's now biting code coverage builds, still, because those import osfile themselves).


When you say "OS.Constants can be accessed by any thread" - can you clarify? Who does off-main-thread access here? I would assume those would end up using their own compartments and instances, because they're on a different thread...

How hard would it be to make the directory service non-mainthread-only? It feels like an odd restriction anyway - the paths should be available everywhere.

Comment 3

[David Teller [:Yoric] - still alive but not very active]

> When you say "OS.Constants can be accessed by any thread" - can you clarify? Who does off-main-thread access here? I would assume those would end up using their own compartments and instances, because they're on a different thread...

ChromeWorker threads have access to `OS.Constants`. I'm not sure I understand your question.

> How hard would it be to make the directory service non-mainthread-only? It feels like an odd restriction anyway - the paths should be available everywhere.

This might become possible once we get rid of XPCOM in add-ons. For the moment, it's impossible, as add-ons can extend it from JS code and this would end up crashing messily. Once that prerequisite is satisfied, exposing it to OS.Constants will still require some work, but this should be feasible.

Comment 4

[Beth Rennie [:beth] (she/her)]

OSFile is being replaced with IOUtils and PathUtils. However, since we are doing some path caching in PathUtils, we should double check that we are only initializing its tempdir members after SetUpSandboxEnvironment

Comment 5

[Beth Rennie [:beth] (she/her)]

Mass move of open Toolkit :: IOUtils and PathUtils bugs to Core :: XPCOM

Comment 6

[:Gijs (away/busy until Mar 10)]

:mccr8, any chance you have a sense of whether this issue actually applies to PathUtils, 8 years later? 🙂

Comment 7

[Beth Rennie [:beth] (she/her)]

:mccr8, SetUpSandboxEnvironment does not seem to be a thing anymore so I don't know what I should be looking for.

Comment 8

[Andrew McCreight [:mccr8]]

Let's just close this. I'm sure there's some equivalent thing, but if it hasn't been a problem for 5 years, eh it is fine.