985197 - Navigator should not entrain all the DeviceStorage objects

Status: RESOLVED INCOMPLETE

(Core :: DOM: Device Interfaces, defect)

Comment 1

[Tapas[:tkundu on #b2g/gaia/memshrink/gfx] (always NI me)]

(v1.3? as this bug blocks a v1.3+ bug)

Comment 2

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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.

Comment 3

[Preeti Raghunath(:Preeti)]

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.

Comment 4

[Jason Smith [:jsmith]]

Kyle - We're having some trouble trying to triage this. Do you need this bug resolved to resolve the blocking bug?

Comment 5

[Tapas[:tkundu on #b2g/gaia/memshrink/gfx] (always NI me)]

(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

Comment 6

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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.

Comment 7

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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.

Comment 8

[Andreas Gal :gal]

We need an owner for this. Instructions are in comment 7.

Comment 9

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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 :/

Comment 10

[Nathan Froyd [:froydnj]]

(In reply to Kyle Huey [:khuey] ([e-mail]) 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?

Comment 11

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

They could even be raw pointers if we clean out array entries from the destructor.

Comment 12

[Nathan Froyd [:froydnj]]

Attachment: make Navigator hold on to nsDOMDeviceStorage objects weakly

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.

Comment 13

[Nathan Froyd [:froydnj]]

(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?

Comment 14

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Looks like you didn't change the QI for nsDOMDeviceStorage to advertise nsISupportsWeakReference.

Comment 15

[Nathan Froyd [:froydnj]]

Attachment: make Navigator hold on to nsDOMDeviceStorage objects weakly

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.

Comment 16

[Nathan Froyd [:froydnj]]

(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 17

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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 :(

Comment 18

[Nathan Froyd [:froydnj]]

(In reply to Kyle Huey [:khuey] ([e-mail]) 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?

Comment 19

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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.

Comment 20

[James Zhang (Spreadtrum)]

Hi Danny, can we try this patch?

Comment 21

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Unfortunately this patch breaks the API.  Fixing this isn't really possible without redesigning the device storgae API :(

Comment 22

[Dave Hylands [:dhylands]]

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.

Comment 23

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

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.