1234192 - OSFileSystem looks scary since it is threadsafe object but keeps a strong pointer to main-thread only mWindow

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Olli Pettay [:smaug][[e-mail]]]

http://mxr.mozilla.org/mozilla-central/source/dom/filesystem/OSFileSystem.h looks scary.
Class which has strong pointer to cycle collectable (and not ever traversed or unlinked - so leaking ?)
object inhering threadsafe refcounting from the base class.
Does anything guarantee the object is always destroyed in the main thread?
Or perhaps OSFileSystem is always used just in the main thread even though there is threadsafe refcounting?
But don't we anyhow end up leaking?

Comment 1

[Andrea Marchesini [:baku]]

Attachment: fsb.patch

I didn't use nsProxyRelease because it doesn't work properly with NS_INLINE_DECL_REFCOUNTING classes.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

But why does the patch fix the possible leak? We still keep strong ref to a Window and not traverse/unlink it.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8702562 [details] [diff] [review]
fsb.patch

This should be fine for the security issue, but I think we should fix the leak issue in this bug too.

Comment 4

[Andrea Marchesini [:baku]]

Attachment: fsb2.patch

Comment 5

[Andrea Marchesini [:baku]]

I landed these 2 patches because the OSFileSystem obj is used only in nightly and protected behind pref.

Comment 6

[Phil Ringnalda (:philor)]

Backed out in https://treeherder.mozilla.org/logviewer.html#?job_id=19094197&repo=mozilla-inbound for the b2g debug assertion failure in test_fs_createFile.html, https://treeherder.mozilla.org/logviewer.html#?job_id=19094197&repo=mozilla-inbound

Comment 7

[Phil Ringnalda (:philor)]

Not just debug, a consistent crash in b2g opt, and not just b2g, an intermittent crash or assertion on desktop.

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/952139e6c454
https://hg.mozilla.org/mozilla-central/rev/cb7ddeee6364

Comment 9

[Andrew McCreight [:mccr8]]

Even though this is disabled in Nightly, I'm going to mark it sec-high for completeness.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

This is my bad. Sorry!
We fixed the security issue, but the traversing/unlinking is now wrong.
mFileSystem is owned by several different Directories, so we can just go from
Directory->fs->mWindow.

(This is all still not enabled-by-default-expect-in-Nightly code)

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

I hadn't realized the same FileSystem object is being reused, but DataTransfer certainly does that.

Comment 12

[Andrea Marchesini [:baku]]

> (This is all still not enabled-by-default-expect-in-Nightly code)

I'm removing this 'reuse' thing in another patch that you are reviewing.
But if you prefer I submit a patch for that here.

Comment 13

[Andrea Marchesini [:baku]]

Attachment: reuse.patch

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8707395 [details] [diff] [review]
reuse.patch

Thanks.

This is still a bit fragile. We should somehow be able to guarantee fs isn't reused.

Would it be possible have an Init method on Directory where you pass window and dirname and it creates OSFileSystem which the Directory uses.
(I'm trying to recall why FileSystem needs a pointer to the global)

Comment 15

[Andrea Marchesini [:baku]]

> Would it be possible have an Init method on Directory where you pass window
> and dirname and it creates OSFileSystem which the Directory uses.
> (I'm trying to recall why FileSystem needs a pointer to the global)

The global is used to create objects by the tasks. Currently we have a window, but as you know, in other patches I'm moving it to a nsISupports.

Comment 16

[Andrea Marchesini [:baku]]

I'll make your proposal in one of the other patches you are reviewing.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/061115751346