Follow-up Bug 1286717 Comment 217.

This looks a major defect that we want to fix it in this release. Shawn, are you going to take this?

Out of curiosity, would it be able to have a test case for this?

Comment on attachment 8860792 [details] [diff] [review] Bug 1358767 - Make PersistentStoragePermissionRequest a cycle collected object in StorageManager Review of attachment 8860792 [details] [diff] [review]: ----------------------------------------------------------------- I'd like to review again after the following comments are addressed. ::: dom/quota/StorageManager.cpp @@ +11,5 @@ > #include "mozilla/dom/StorageManagerBinding.h" > #include "mozilla/dom/WorkerPrivate.h" > #include "mozilla/ErrorResult.h" > #include "nsContentPermissionHelper.h" > +#include "nsCycleCollectionParticipant.h" This has already been already included in StorageManager.h @@ +198,2 @@ > NS_DECL_NSICONTENTPERMISSIONREQUEST > + NS_DECL_CYCLE_COLLECTION_CLASS_AMBIGUOUS(PersistentStoragePermissionRequest, You don't need _AMBIGUOUS one unless unless it inherits from multiple interfaces: https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Interfacing_with_the_XPCOM_cycle_collector#How_to_make_your_classes_participate @@ +686,5 @@ > +NS_IMPL_CYCLE_COLLECTION(PersistentStoragePermissionRequest, mWindow, mPromise) > + > +NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(PersistentStoragePermissionRequest) > + NS_INTERFACE_MAP_ENTRY(nsIContentPermissionRequest) > + NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, nsIContentPermissionRequest) ditto: NS_INTERFACE_MAP_ENTRY(nsISupports)

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #5) > Out of curiosity, would it be able to have a test case for this? I'm trying to do it. But currently I don't see easy way to do it. I need more time to think about tests in this case.

Comment on attachment 8860836 [details] [diff] [review] Bug 1358767 - Make PersistentStoragePermissionRequest a cycle collected object in StorageManager Fix addressed issues in Comment 6.

Comment on attachment 8860836 [details] [diff] [review] Bug 1358767 - Make PersistentStoragePermissionRequest a cycle collected object in StorageManager Review of attachment 8860836 [details] [diff] [review]: ----------------------------------------------------------------- With the assumption as what NotificationPermissionRequest has done that there is no need to have PersistentStoragePermissionRequest::mRequester joined this cycle collection.

(In reply to Bevis Tseng[:bevistseng][:btseng] from comment #11) > Comment on attachment 8860836 [details] [diff] [review] > Bug 1358767 - Make PersistentStoragePermissionRequest a cycle collected > object in StorageManager > > Review of attachment 8860836 [details] [diff] [review]: > ----------------------------------------------------------------- > > With the assumption as what NotificationPermissionRequest has done that > there is no need to have PersistentStoragePermissionRequest::mRequester > joined this cycle collection. Yeah, I think it's not necessary.

Comment on attachment 8860836 [details] [diff] [review] Bug 1358767 - Make PersistentStoragePermissionRequest a cycle collected object in StorageManager Review of attachment 8860836 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/quota/StorageManager.cpp @@ -192,5 @@ > > nsresult > Start(); > > - NS_DECL_THREADSAFE_ISUPPORTS Going from threadsafe to cycle collecting isn't safe in general, but I think it was threadsafe for now reason. @@ +688,5 @@ > + NS_INTERFACE_MAP_ENTRY(nsISupports) > +NS_INTERFACE_MAP_END > + > +NS_IMPL_CYCLE_COLLECTING_ADDREF(PersistentStoragePermissionRequest) > +NS_IMPL_CYCLE_COLLECTING_RELEASE(PersistentStoragePermissionRequest) Nit: I know that you followed the code in Notification.cpp, but I think the order should be: NS_IMPL_CYCLE_COLLECTING_ADDREF(...) NS_IMPL_CYCLE_COLLECTING_RELEASE(...) NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(...) ... NS_INTERFACE_MAP_END NS_IMPL_CYCLE_COLLECTION(...) @@ -698,5 @@ > return nsContentPermissionUtils::AskPermission(this, mWindow); > } > > -NS_IMPL_ISUPPORTS(PersistentStoragePermissionRequest, > - nsIContentPermissionRequest) Nit: the new stuff should go here to follow the order in the declaration (|NS_DECL_CYCLE_COLLECTING_ISUPPORTS| is places after |nsresult Start()|)

Comment on attachment 8860836 [details] [diff] [review] Bug 1358767 - Make PersistentStoragePermissionRequest a cycle collected object in StorageManager Review of attachment 8860836 [details] [diff] [review]: ----------------------------------------------------------------- Don't forget to add reviewer to the commit message.

Comment on attachment 8860896 [details] [diff] [review] Bug 1358767 - Make PersistentStoragePermissionRequest a cycle collected object in StorageManager, r=janv,bevistseng Review of attachment 8860896 [details] [diff] [review]: ----------------------------------------------------------------- Nice!

Pushed by shuang@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/b57e9a48a8bb Make PersistentStoragePermissionRequest a cycle collected object in StorageManager, r=janv,bevistseng