1167083 - Windows low integrity temp clean-up code leaks an nsIFile on shutdown.

Status: RESOLVED FIXED

(Core :: Security: Process Sandboxing, defect)

Description

[Bob Owen (:bobowen)]

I hadn't realised that:

unused << functionThatReturnsAlready_Addrefed();

Just leaks the pointer because it doesn't release the reference.

I also noticed that if someone has turned off e10s or reduced the sandbox level, then the old MozTemp clean-up code still might not run either.

Comment 1

[Bob Owen (:bobowen)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3d07d6366b8d

Just to be clear, I introduced this leak in a bit of refactoring in response to a review, so it wasn't there when the code was originally reviewed.

Comment 2

[Bob Owen (:bobowen)]

Attachment: Fix memory leak in Windows low integrity temp clean up code.

Sorry about this Bill.

This should fix the memory leak and really ensure that the old MozTemp clean-up code always runs on Vista or later.

Comment 3

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/31662149dab4

Comment 4

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/228109b9b9ab

Comment 5

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

backed out for xperf issue:
TEST-UNEXPECTED-FAIL | mainthreadio | File '{appdata}\locallow\mozilla' was accessed and we were not expecting it: {'Count': 1, 'Duration': 0.081791, 'RunCount': 1}

I would like to have vladan or aklotz determine if this is ok to add to the talos whitelist file:
http://hg.mozilla.org/build/talos/diff/39c9c9b33cfc/talos/mtio-whitelist.json

if so we can add it and update talos.json in tree when this lands next time.

Comment 6

[Bob Owen (:bobowen)]

(In reply to Joel Maher (:jmaher) from comment #5)
> backed out for xperf issue:
> TEST-UNEXPECTED-FAIL | mainthreadio | File '{appdata}\locallow\mozilla' was
> accessed and we were not expecting it: {'Count': 1, 'Duration': 0.081791,
> 'RunCount': 1}
> 
> I would like to have vladan or aklotz determine if this is ok to add to the
> talos whitelist file:
> http://hg.mozilla.org/build/talos/diff/39c9c9b33cfc/talos/mtio-whitelist.json
> 
> if so we can add it and update talos.json in tree when this lands next time.

Thanks jmaher.

In e10s mode this code will also create a directory under AppData\LocalLow\Mozilla on start-up and delete it on shutdown.

That code has already landed, but probably hasn't triggered anything as we're not running Talos with e10s.

Comment 7

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

we do run this test on mozilla-central in e10s mode for pgo builds:
https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&filter-searchStr=pgo%20xperf&exclusion_profile=false

these are hidden by default as they don't meet the criteria of jobs which are scheduled frequently enough.

low and behold we see the above mentioned file as well as:
'{appdata}\locallow\mozilla\temp-{5b4c46ee-cd88-4c3f-869f-1faa32a07611}'

Comment 8

[Vladan Djeric (:vladan)]

So if I understand this correctly, this patch fixes the code for cleaning up low-privilege temp files so that it runs at shutdown even when we're in non-e10s mode?
I'm guessing these temp files need to be removed because of privacy and disk space concerns? If that's the case, I'm ok with whitelisting this shutdown I/O.

Comment 9

[Bob Owen (:bobowen)]

(In reply to Vladan Djeric (:vladan) -- please needinfo! from comment #8)
> So if I understand this correctly, this patch fixes the code for cleaning up
> low-privilege temp files so that it runs at shutdown even when we're in
> non-e10s mode?
> I'm guessing these temp files need to be removed because of privacy and disk
> space concerns? If that's the case, I'm ok with whitelisting this shutdown
> I/O.

Thanks.
This patch and the one in bug 1166316 are to do with deleting these temp directories.
In order to minimise any problems with locking, I think that this needs to happen fairly late in shutdown, which is why I put it where I did.

However, the patch for bug 1162327 added the creation of this temp directory on start-up, which now I know that these checks exist, I suspect people will be less willing to white-list.

It looks like content processes are all started using the MessageLoop returned by XRE_GetIOMessageLoop, so I assume that if I post the file access parts of my change to that loop, I'm guaranteed that it will happen before any content processes start.

Vladan - does that make sense?
As the start-up IO is not directly related to this bug, I'll do that in (yet another) follow-up, if that's OK?

Joel - for white-listing the shutdown IO is it just a matter of adding |"{AppData}\\locallow\\mozilla": {},| to that list?
Can we distinguish between shutdown IO and other IO?
Does this work like alos/xtalos/xperf_whitelist.json, where we can add mincount and maxcount?
Can I test this on try?

Sorry for all the questions.

Comment 10

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

As a side-note: if you want things to happen late during shutdown, don't hesitate to look at AsyncShutdown.

Comment 11

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

Thanks for asking more questions!

mtio_whitelist.json vs xperf_whitelist.json;  these are two different systems and they both report failures and turn the 'x' job orange on treeherder.  the mtio-whitelist.json file looks for filenames and total access time while on the mainthread.  I believe this is during startup only whereas the full xperf stuff detects access counts during the entire browser cycle.  We would just need to add "{AppData}\\locallow\\mozilla": {}" to mtio-whitelist.json.  

** keep in mind on m-c where we run talos e10s, we see '{appdata}\locallow\mozilla\temp-{5b4c46ee-cd88-4c3f-869f-1faa32a07611}' accessed.  I assume that is a dynamic guid- we can hack that if needed to allow a wildcard.


Here are some steps to run on try:
https://wiki.mozilla.org/Buildbot/Talos/Running#Testing_changes_to_talos_properly

an example try push that I have using a custom talos repo (http://hg.mozilla.org/users/jmaher_mozilla.com/tpain):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9e017bea3f28

Comment 12

[Vladan Djeric (:vladan)]

(In reply to Bob Owen (:bobowen) from comment #9)
> Vladan - does that make sense?
> As the start-up IO is not directly related to this bug, I'll do that in (yet
> another) follow-up, if that's OK?

Sure, we can discuss it in another bug

(In reply to Joel Maher (:jmaher) from comment #11)
> the mtio-whitelist.json file looks for filenames and total
> access time while on the mainthread. I believe this is during startup only
< snip >  
> We would just need to add "{AppData}\\locallow\\mozilla": {}" to
> mtio-whitelist.json.  

This bug isn't for whitelisting startup I/O though
> ** keep in mind on m-c where we run talos e10s, we see
> '{appdata}\locallow\mozilla\temp-{5b4c46ee-cd88-4c3f-869f-1faa32a07611}'
> accessed.  I assume that is a dynamic guid- we can hack that if needed to
> allow a wildcard.
> 
> 
> Here are some steps to run on try:
> https://wiki.mozilla.org/Buildbot/Talos/
> Running#Testing_changes_to_talos_properly
> 
> an example try push that I have using a custom talos repo
> (http://hg.mozilla.org/users/jmaher_mozilla.com/tpain):
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=9e017bea3f28

Comment 13

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

vladan, is there anything we need to do in talos to whitelist this?  I am a bit confused, apologies.

Comment 14

[Vladan Djeric (:vladan)]

As far as I know, the xperf test detects startup I/O only, and its whitelist is xperf_whitelist.json
The other main-thread I/O Talos test (undocumented/unnamed?) checks for main-thread I/O at any point during the TP5 Talos tests and its whitelist is mtio-whitelist.json.

So I think we should add {appdata}\locallow\mozilla to the mtio-whitelist.json file in this bug

Comment 15

[Bob Owen (:bobowen)]

Attachment: Add AppData\LocalLow\Mozilla to the talos mtio list, for Temp directory clean-up on shutdown.

As pointed out in the comments this seems to do the trick.

I've filed bug 1169208, for sorting out the T-e10s(x) failures.

As far as I can tell the mainthreadio tests ignore the start-up stage.
Maybe because they are covered by the other ones, (I'm not sure how the stage is determined in the logs, or whether both tests use the same definition).

The stage only seems to be used for skipping the start-up stage at the moment, you can't specify the stage in the whitelist as far as I can tell.

Try push without talos change:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=53111a702b70

Try push with talos change:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4223f9b79d56

Comment 16

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

Comment on attachment 8612219 [details] [diff] [review]
Add AppData\LocalLow\Mozilla to the talos mtio list, for Temp directory clean-up on shutdown.

Review of attachment 8612219 [details] [diff] [review]:
-----------------------------------------------------------------

this looks good.  You can land this and just update talos.json to point to the correct revision when you land your code in an integration branch!  There is no need to update the talos.*.zip reference, that is for android only.

Comment 17

[Bob Owen (:bobowen)]

remote:   https://hg.mozilla.org/build/talos/rev/e04283e4f2b2

Thanks Joel.

Comment 18

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/80c50de1a62b

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/80c50de1a62b