Closed Bug 985197 Opened 6 years ago Closed 2 years ago

Navigator should not entrain all the DeviceStorage objects

Categories

(Core :: DOM: Device Interfaces, defect)

defect
Not set

Tracking

()

RESOLVED INCOMPLETE

People

(Reporter: khuey, Unassigned)

References

Details

(Keywords: memory-leak, Whiteboard: [MemShrink:P2][DeviceStorageInvestigate])

Attachments

(1 file, 1 obsolete file)

Whiteboard: [MemShrink] → [MemShrink:P2]
(v1.3? as this bug blocks a v1.3+ bug)
blocking-b2g: --- → 1.3?
So we're on the same page, every memory leak that we have could conceivably block bug 981871.  That doesn't mean they all need to block 1.3.
Tapas,

Is QC blocked by this bug in 1.3?

Please confirm if that's the case and we'll reconsider. I agree with Kyle here.
Flags: needinfo?(tkundu)
Kyle - We're having some trouble trying to triage this. Do you need this bug resolved to resolve the blocking bug?
Flags: needinfo?(khuey)
(In reply to Jason Smith [:jsmith] from comment #4)
> Kyle - We're having some trouble trying to triage this. Do you need this bug
> resolved to resolve the blocking bug?

I also agree with him as he is debugging bug 981871. I guess that Kyle wants bug 985042 and bug 985827 to be landed in v1.3 for bug 981871 [1] .

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=981871#c77
Flags: needinfo?(tkundu)
Whether or not we need this for 1.3 is really up to QC and when they're willing to call bug 981871 fixed.  If they're happy with the numbers after bug 985827 is fixed then we don't need this.  But if they're not, this is the next thing on my list to do.
Flags: needinfo?(khuey)
To fix this bug, we need to make nsDOMDeviceStorage inherit from nsSupportsWeakReference.  Then we can set the weak reference argument to nsIObserverService::AddObserver to true and move the RemoveObserver calls and UnregisterForSDCardChanges from Shutdown() to the destructor.  We should also be able to move the filesystem handling to the destructor too, since all DeviceStorageFileSystem::Shutdown() does is null out a raw pointer.  Once we've done all of that, nsDOMDeviceStorage::Shutdown isn't needed anymore and we can remove it and the code that calls it in Navigator, including the array of strong references to all the nsDOMDeviceStorages ever created.
We need an owner for this. Instructions are in comment 7.
blocking-b2g: 1.3? → 1.3+
Actually, I wrote those instructions too hastily.  We fire DOM events off of those observer notifications, so if we make them weak we might get GCd and not fire events that we need to.  Untangling that will be hard ...

Perhaps a good place to start is just to make the references from the Navigator to the nsDOMDeviceStorages weak.  That will allow the nsDOMDeviceStorage to be GCd if we never did anything that caused us to add observers.  Fixing everything probably requires redesigning the API :/
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #9)
> Actually, I wrote those instructions too hastily.  We fire DOM events off of
> those observer notifications, so if we make them weak we might get GCd and
> not fire events that we need to.  Untangling that will be hard ...
> 
> Perhaps a good place to start is just to make the references from the
> Navigator to the nsDOMDeviceStorages weak.  That will allow the
> nsDOMDeviceStorage to be GCd if we never did anything that caused us to add
> observers.  Fixing everything probably requires redesigning the API :/

So this is just making Navigator hold nsIWeakReferences instead of concrete nsDOMDeviceStorage?
They could even be raw pointers if we clean out array entries from the destructor.
This patch makes the compiler happy and I think implements what Kyle was
talking about.  I'm not sure that it actually works correctly; running
it through try to find out.
(In reply to Nathan Froyd (:froydnj) from comment #12)
> This patch makes the compiler happy and I think implements what Kyle was
> talking about.  I'm not sure that it actually works correctly; running
> it through try to find out.

It looks like this patch makes some of the devicestorage tests time out.  Kyle, do you see anything obviously wrong with the patch?
Flags: needinfo?(khuey)
Looks like you didn't change the QI for nsDOMDeviceStorage to advertise nsISupportsWeakReference.
Flags: needinfo?(khuey)
Thanks Kyle, that looks like it was the problem.  Tests are looking
greener on try (still waiting for some debug tests to finish), but at
least the opt tests are no longer crashing.
Attachment #8395702 - Attachment is obsolete: true
Attachment #8396082 - Flags: review?(khuey)
(In reply to Nathan Froyd (:froydnj) from comment #15)
> Thanks Kyle, that looks like it was the problem.  Tests are looking
> greener on try (still waiting for some debug tests to finish), but at
> least the opt tests are no longer crashing.

Looking pretty green on debug too: https://tbpl.mozilla.org/?tree=Try&rev=408708435cd6
Comment on attachment 8396082 [details] [diff] [review]
make Navigator hold on to nsDOMDeviceStorage objects weakly

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

r=me with the weak refs in the observer service dropped

Have you verified that if we do navigator.getDeviceStorage("foo"); and don't use the result that it gets GCd?

::: dom/devicestorage/nsDeviceStorage.cpp
@@ +1625,5 @@
>                                               aStorageName,
>                                               getter_AddRefs(f));
>    nsCOMPtr<nsIObserverService> obs = mozilla::services::GetObserverService();
> +  obs->AddObserver(this, "file-watcher-update", true);
> +  obs->AddObserver(this, "disk-space-watcher", true);

I don't think we can do this yet.  If these observers are not strong then we could get GCd before expected events fire :(
Attachment #8396082 - Flags: review?(khuey) → review+
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #17)
> ::: dom/devicestorage/nsDeviceStorage.cpp
> @@ +1625,5 @@
> >                                               aStorageName,
> >                                               getter_AddRefs(f));
> >    nsCOMPtr<nsIObserverService> obs = mozilla::services::GetObserverService();
> > +  obs->AddObserver(this, "file-watcher-update", true);
> > +  obs->AddObserver(this, "disk-space-watcher", true);
> 
> I don't think we can do this yet.  If these observers are not strong then we
> could get GCd before expected events fire :(

So this seems like we haven't really made any progress, then, because the observer service is still holding strong refs to our DeviceStorage things.  And so DeviceStorage objects still get entrained for all time.

It would be nice if we didn't always add the observer service references, but AFAICS, the observer refs are added in SetRootDirectoryForType, which is called from Init, which means that the observer service refs get added always.

Am I missing something?
Flags: needinfo?(khuey)
Yeah, I was afraid of that.  I think we should avoid trying to rearchitect this in the 1.3 time frame.  If bug 981871 is not fixed yet we will pursue other avenues to further reduce memory usage.
No longer blocks: 981871
blocking-b2g: 1.3+ → ---
Flags: needinfo?(khuey)
See Also: → 1031977
Hi Danny, can we try this patch?
Blocks: 1031977
Unfortunately this patch breaks the API.  Fixing this isn't really possible without redesigning the device storgae API :(
Its never been clear to me why we need to retain more than one nsIDOMDeviceStorage object per storageType/Volume (per app). So if I call navigator.getDeviceStorage('pictures') twice in a row (from the same app), is there a reason it can't return the same object rather than returning a new object.
Whiteboard: [MemShrink:P2] → [MemShrink:P2][DeviceStorageInvestigate]
Assignee: khuey → nobody
Cleaning up Device Interfaces component, and mass-marking old FxOS bugs as incomplete.

If any of these bugs are still valid, please let me know.
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → INCOMPLETE
You need to log in before you can comment on or make changes to this bug.