Closed
Bug 1358767
Opened 7 years ago
Closed 7 years ago
Make PersistentStoragePermissionRequest a cycle collected object in StorageManager
Categories
(Core :: Storage: Quota Manager, defect, P1)
Core
Storage: Quota Manager
Tracking
()
RESOLVED
FIXED
mozilla55
People
(Reporter: shawnjohnjr, Assigned: shawnjohnjr)
References
Details
Attachments
(1 file, 4 obsolete files)
Follow-up Bug 1286717 Comment 217.
Assignee | ||
Updated•7 years ago
|
Summary: Traverses/unlinks mWindow for PersistentStoragePermissionRequest in StorageManager → Make PersistentStoragePermissionRequest a cycle collected object in StorageManager
Comment 1•7 years ago
|
||
This looks a major defect that we want to fix it in this release. Shawn, are you going to take this?
Flags: needinfo?(shuang)
Priority: -- → P1
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → shuang
Flags: needinfo?(shuang)
Assignee | ||
Updated•7 years ago
|
status-firefox55:
--- → affected
tracking-firefox55:
--- → ?
Assignee | ||
Comment 2•7 years ago
|
||
Assignee | ||
Updated•7 years ago
|
Attachment #8860791 -
Attachment is obsolete: true
Assignee | ||
Comment 3•7 years ago
|
||
Assignee | ||
Comment 4•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a1745eb467987598982454f284612758b0c30332
Assignee | ||
Updated•7 years ago
|
Attachment #8860792 -
Flags: feedback?(btseng)
Comment 5•7 years ago
|
||
Out of curiosity, would it be able to have a test case for this?
Comment 6•7 years ago
|
||
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)
Attachment #8860792 -
Flags: feedback?(btseng)
Assignee | ||
Updated•7 years ago
|
Attachment #8860792 -
Attachment is obsolete: true
Assignee | ||
Comment 7•7 years ago
|
||
Assignee | ||
Comment 8•7 years ago
|
||
Assignee | ||
Updated•7 years ago
|
Attachment #8860835 -
Attachment is obsolete: true
Assignee | ||
Comment 9•7 years ago
|
||
(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.
Assignee | ||
Comment 10•7 years ago
|
||
Comment on attachment 8860836 [details] [diff] [review] Bug 1358767 - Make PersistentStoragePermissionRequest a cycle collected object in StorageManager Fix addressed issues in Comment 6.
Attachment #8860836 -
Flags: feedback?(btseng)
Comment 11•7 years ago
|
||
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.
Attachment #8860836 -
Flags: feedback?(btseng) → feedback+
Assignee | ||
Comment 12•7 years ago
|
||
(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.
Assignee | ||
Updated•7 years ago
|
Attachment #8860836 -
Flags: review?(jvarga)
Comment 13•7 years ago
|
||
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()|)
Attachment #8860836 -
Flags: review?(jvarga) → review+
Comment 14•7 years ago
|
||
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.
Assignee | ||
Updated•7 years ago
|
Attachment #8860836 -
Attachment is obsolete: true
Assignee | ||
Comment 15•7 years ago
|
||
Comment 16•7 years ago
|
||
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!
Attachment #8860896 -
Flags: review+
Comment 17•7 years ago
|
||
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
Comment 18•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/b57e9a48a8bb
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Updated•7 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•