Closed Bug 1234192 Opened 7 years ago Closed 7 years ago
System looks scary since it is threadsafe object but keeps a strong pointer to main-thread only m Window
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?
I didn't use nsProxyRelease because it doesn't work properly with NS_INLINE_DECL_REFCOUNTING classes.
Assignee: nobody → amarchesini
Attachment #8702562 - Flags: review?(bugs)
But why does the patch fix the possible leak? We still keep strong ref to a Window and not traverse/unlink it.
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.
Attachment #8702562 - Flags: review?(bugs) → review+
Attachment #8702676 - Flags: review?(bugs) → review+
I landed these 2 patches because the OSFileSystem obj is used only in nightly and protected behind pref.
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
Not just debug, a consistent crash in b2g opt, and not just b2g, an intermittent crash or assertion on desktop.
Even though this is disabled in Nightly, I'm going to mark it sec-high for completeness.
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)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
I hadn't realized the same FileSystem object is being reused, but DataTransfer certainly does that.
> (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 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)
Attachment #8707395 - Flags: review?(bugs) → review+
> 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.
I'll make your proposal in one of the other patches you are reviewing.
You need to log in before you can comment on or make changes to this bug.