Closed Bug 1641231 Opened 3 years ago Closed 3 years ago

Follow pointer guideline for dom/quota/ActorsParent.cpp

Categories

(Core :: Storage: Quota Manager, enhancement, P3)

Product:
Core
Component:
Storage: Quota Manager
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: tt, Assigned: tt)

Details

Attachments

(10 files)

Bug 1641231 - Replace new operator with MakeRefPtr for constructing actor classes;
47 bytes, text/x-phabricator-request
Details | Review
Bug 1641231 - Reduce the usage for passing raw pointers in RegisterNormalOriginOp/UnregisterNormalOriginOp;
47 bytes, text/x-phabricator-request
Details | Review
Bug 1641231 - Do not pass a raw pointer only for dereferencing for functions in actor classes;
47 bytes, text/x-phabricator-request
Details | Review
Bug 1641231 - Use Result instead of an out parameter for QuotaUsageRequestBase::GetUsageForOrigin;
47 bytes, text/x-phabricator-request
Details | Review
Bug 1641231 - Split GetUsageForOrigin into a smaller function;
47 bytes, text/x-phabricator-request
Details | Review
Bug 1641231 - Replace raw pointers as functions arguments in DirectoryLockImpl;
47 bytes, text/x-phabricator-request
Details | Review
Bug 1641231 - Use NotNull for DirectoryLockImpl's member variables;
47 bytes, text/x-phabricator-request
Details | Review
Bug 1641231 - Use InitializedOnce for DirectoryLockImpl::mOpenListener;
47 bytes, text/x-phabricator-request
Details | Review
Bug 1641231 - Use FlippedOnce for DirectoryLockImpl::mInvalidate;
47 bytes, text/x-phabricator-request
Details | Review
Bug 1641231 - Reduce raw pointers as functions arguments for DirectoryLock related functions in QuotaManager;
47 bytes, text/x-phabricator-request
Details | Review
No description provided.

I would like to start applying that with actor classes and then class for directory locks.

Attachment #9152108 - Attachment description: Bug 1641231 - Use Result as out parameter for QuotaUsageRequestBase::GetUsageForOrigin; → Bug 1641231 - Use Result instead of an out parameter for QuotaUsageRequestBase::GetUsageForOrigin;

I plan to apply the guideline to some other classes in dom/quota/ActorsParent.cpp

Keywords: leave-open
Pushed by ttung@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9466bba20117
Replace new operator with MakeRefPtr for constructing actor classes; r=dom-workers-and-storage-reviewers,sg
https://hg.mozilla.org/integration/autoland/rev/7106175afc6a
Reduce the usage for passing raw pointers in RegisterNormalOriginOp/UnregisterNormalOriginOp; r=dom-workers-and-storage-reviewers,sg
https://hg.mozilla.org/integration/autoland/rev/f0cdc44b0023
Do not pass a raw pointer only for dereferencing for functions in actor classes; r=dom-workers-and-storage-reviewers,sg
https://hg.mozilla.org/integration/autoland/rev/cf429262e727
Use Result instead of an out parameter for QuotaUsageRequestBase::GetUsageForOrigin; r=dom-workers-and-storage-reviewers,sg
https://hg.mozilla.org/integration/autoland/rev/738524c19b7f
Split GetUsageForOrigin into a smaller function; r=dom-workers-and-storage-reviewers,sg,janv

I plan to make DirectoryLock as a SafeRefPtr and update DirectoryLockAcquired

Attachment #9153843 - Attachment description: Bug 1641231 - Use NotNull for DirectoryLockImpl::mQuotaManager; → Bug 1641231 - Use NotNull for DirectoryLockImpl's member variables;
Attachment #9153846 - Attachment description: Bug 1641231 - Reduce raw pointers as functions arguments for DriectoryLock related functions in QuotaManager; → Bug 1641231 - Reduce raw pointers as functions arguments for DirectoryLock related functions in QuotaManager;
Summary: Follow pointer guideline for actor classes in dom/quota/ActorsParent.cpp → Follow pointer guideline for dom/quota/ActorsParent.cpp
Pushed by ttung@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/84668b82ebee
Replace raw pointers as functions arguments in DirectoryLockImpl; r=dom-workers-and-storage-reviewers,sg
https://hg.mozilla.org/integration/autoland/rev/203e3ec45022
Use NotNull for DirectoryLockImpl's member variables; r=dom-workers-and-storage-reviewers,sg
https://hg.mozilla.org/integration/autoland/rev/125533344189
Use InitializedOnce for DirectoryLockImpl::mOpenListener; r=dom-workers-and-storage-reviewers,janv,sg
https://hg.mozilla.org/integration/autoland/rev/3c4db99ba498
Use FlippedOnce for DirectoryLockImpl::mInvalidate; r=dom-workers-and-storage-reviewers,sg
https://hg.mozilla.org/integration/autoland/rev/f91248e16c71
Reduce raw pointers as functions arguments for DirectoryLock related functions in QuotaManager; r=dom-workers-and-storage-reviewers,sg

Backed out for assertion failures on FlippedOnce.h and crashes on OpenDirectory

backout: https://hg.mozilla.org/integration/autoland/rev/77ca88c4478a8c1ad318022a7dab4e1964fb8dfe

push: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=f91248e16c71eb35e63a9df4de3ccccf6165b4f0&group_state=expanded&selectedTaskRun=VgYzLaZnREeTXy_bKY4nhg-0

failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=305665839&repo=autoland&lineNumber=58879

[task 2020-06-09T17:20:27.948Z] 17:20:27 INFO - Assertion failure: mValue == Initial, at /builds/worker/workspace/obj-build/dist/include/mozilla/dom/FlippedOnce.h:30
[task 2020-06-09T17:20:27.948Z] 17:20:27 INFO - DEBUG: Starting phase xpcom-will-shutdown
[task 2020-06-09T17:20:27.948Z] 17:20:27 INFO - #01: mozilla::dom::quota::(anonymous namespace)::NormalOriginOperationBase::Open()[/Users/cltbld/tasks/task_1591722249/build/application/Firefox NightlyDebug.app/Contents/MacOS/XUL +0x3c41c4d]
[task 2020-06-09T17:20:27.948Z] 17:20:27 INFO - #02: mozilla::dom::quota::(anonymous namespace)::OriginOperationBase::Run()[/Users/cltbld/tasks/task_1591722249/build/application/Firefox NightlyDebug.app/Contents/MacOS/XUL +0x3c37034]
[task 2020-06-09T17:20:27.948Z] 17:20:27 INFO - DEBUG: Spinning the event loop
[task 2020-06-09T17:20:27.948Z] 17:20:27 INFO - #03: mozilla::dom::quota::(anonymous namespace)::Quota::RecvPQuotaRequestConstructor(mozilla::dom::quota::PQuotaRequestParent*, mozilla::dom::quota::RequestParams const&)[/Users/cltbld/tasks/task_1591722249/build/application/Firefox NightlyDebug.app/Contents/MacOS/XUL +0x3c3f156]
[task 2020-06-09T17:20:27.948Z] 17:20:27 INFO - DEBUG: Finished phase xpcom-will-shutdown
[task 2020-06-09T17:20:27.948Z] 17:20:27 INFO - DEBUG: Starting phase web-workers-shutdown
[task 2020-06-09T17:20:27.948Z] 17:20:27 INFO - #04: mozilla::dom::quota::PQuotaParent::OnMessageReceived(IPC::Message const&)[/Users/cltbld/tasks/task_1591722249/build/application/Firefox NightlyDebug.app/Contents/MacOS/XUL +0xcec8a2]
[task 2020-06-09T17:20:27.948Z] 17:20:27 INFO - DEBUG: Spinning the event loop
[task 2020-06-09T17:20:27.948Z] 17:20:27 INFO - DEBUG: Finished phase web-workers-shutdown
[task 2020-06-09T17:20:27.949Z] 17:20:27 INFO - #05: mozilla::ipc::PBackgroundParent::OnMessageReceived(IPC::Message const&)[/Users/cltbld/tasks/task_1591722249/build/application/Firefox NightlyDebug.app/Contents/MacOS/XUL +0xe85b8e]
[task 2020-06-09T17:20:27.949Z] 17:20:27 INFO - [Child 1406, Main Thread] WARNING: nsAppShell::Exit() called redundantly: file /builds/worker/checkouts/gecko/widget/cocoa/nsAppShell.mm, line 714
[task 2020-06-09T17:20:27.949Z] 17:20:27 INFO - #06: mozilla::ipc::MessageChannel::DispatchAsyncMessage(mozilla::ipc::ActorLifecycleProxy*, IPC::Message const&)[/Users/cltbld/tasks/task_1591722249/build/application/Firefox NightlyDebug.app/Contents/MacOS/XUL +0x9c8ae0]
[task 2020-06-09T17:20:27.949Z] 17:20:27 INFO - #07: mozilla::ipc::MessageChannel::DispatchMessage(IPC::Message&&)[/Users/cltbld/tasks/task_1591722249/build/application/Firefox NightlyDebug.app/Contents/MacOS/XUL +0x9c676a]
[task 2020-06-09T17:20:27.949Z] 17:20:27 INFO - #08: mozilla::ipc::MessageChannel::RunMessage(mozilla::ipc::MessageChannel::MessageTask&)[/Users/cltbld/tasks/task_1591722249/build/application/Firefox NightlyDebug.app/Contents/MacOS/XUL +0x9c7485]
[task 2020-06-09T17:20:27.949Z] 17:20:27 INFO - #09: mozilla::ipc::MessageChannel::MessageTask::Run()[/Users/cltbld/tasks/task_1591722249/build/application/Firefox NightlyDebug.app/Contents/MacOS/XUL +0x9c7b4a]
[task 2020-06-09T17:20:27.949Z] 17:20:27 INFO - #10: nsThread::ProcessNextEvent(bool, bool*)[/Users/cltbld/tasks/task_1591722249/build/application/Firefox NightlyDebug.app/Contents/MacOS/XUL +0x18b49c]
[task 2020-06-09T17:20:27.954Z] 17:20:27 INFO - #11: NS_ProcessNextEvent(nsIThread*, bool)[/Users/cltbld/tasks/task_1591722249/build/application/Firefox NightlyDebug.app/Contents/MacOS/XUL +0x190f1c]
[task 2020-06-09T17:20:27.954Z] 17:20:27 INFO - #12: mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*)[/Users/cltbld/tasks/task_1591722249/build/application/Firefox NightlyDebug.app/Contents/MacOS/XUL +0x9cd8c1]
[task 2020-06-09T17:20:27.954Z] 17:20:27 INFO - ###!!! [Parent][RunMessage] Error: Channel closing: too late to send/recv, messages will be lost
[task 2020-06-09T17:20:27.954Z] 17:20:27 INFO - #13: MessageLoop::Run()[/Users/cltbld/tasks/task_1591722249/build/application/Firefox NightlyDebug.app/Contents/MacOS/XUL +0x95997e]
[task 2020-06-09T17:20:27.954Z] 17:20:27 INFO - #14: nsThread::ThreadFunc(void*)[/Users/cltbld/tasks/task_1591722249/build/application/Firefox NightlyDebug.app/Contents/MacOS/XUL +0x18744a]
[task 2020-06-09T17:20:27.954Z] 17:20:27 INFO - #15: _pt_root[/Users/cltbld/tasks/task_1591722249/build/application/Firefox NightlyDebug.app/Contents/MacOS/libnss3.dylib +0x1842d1]
[task 2020-06-09T17:20:27.954Z] 17:20:27 INFO - #16: _pthread_body[/usr/lib/system/libsystem_pthread.dylib +0x32eb]
[task 2020-06-09T17:20:27.955Z] 17:20:27 INFO - #17: _pthread_start[/usr/lib/system/libsystem_pthread.dylib +0x6249]
[task 2020-06-09T17:20:28.060Z] 17:20:28 INFO - Exiting due to channel error.
[task 2020-06-09T17:20:28.104Z] 17:20:28 INFO - [Child 1406, Main Thread] WARNING: Extra shutdown CC: 'i < NORMAL_SHUTDOWN_COLLECTIONS', file /builds/worker/checkouts/gecko/xpcom/base/nsCycleCollector.cpp, line 3359

Flags: needinfo?(ttung)

It's toolkit/components/cleardata/tests/marionette/test_service_worker_at_shutdown.py. I only verified all tests for dom/quota pass. Sorry for that. I will check this tomorrow.

Flags: needinfo?(ttung)

It's because we set mInvalidate to true twice in this test.

So, I think I misunderstood the design of DirectoryLockImpl::Invalidate(). I thought we shouldn't invalidate a lock more than one time, but, actually, the code doesn't guarantee that and Invalidate() is designed to be like ensuing mInvalidate is set.
We can make it like:

if (!mInvalidate) {
 mInvalidated = true;
}

, but I believe abandon D78067 is a better way to do since it doesn't match the semantic of FlippedOnce.

Attachment #9153845 - Attachment is obsolete: true

(In reply to Tom Tung [:tt, :ttung] from comment #19)

It's because we set mInvalidate to true twice in this test.

So, I think I misunderstood the design of DirectoryLockImpl::Invalidate(). I thought we shouldn't invalidate a lock more than one time, but, actually, the code doesn't guarantee that and Invalidate() is designed to be like ensuing mInvalidate is set.
We can make it like:

if (!mInvalidate) {
 mInvalidated = true;
}

, but I believe abandon D78067 is a better way to do since it doesn't match the semantic of FlippedOnce.

Actually I think it does match the design. You just need to use EnsureFlipped instead of Flip. FlippedOnce still ensures that it never gets back to false once it was set to true.

(In reply to Simon Giesecke [:sg] [he/him] from comment #21)

Actually I think it does match the design. You just need to use EnsureFlipped instead of Flip. FlippedOnce still ensures that it never gets back to false once it was set to true.

Ah, okay. I wasn't aware of EnsureFlipped. Will check that and maybe revert the change I just made. Thanks for the suggestion!

Attachment #9153845 - Attachment is obsolete: false
Pushed by ttung@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/62076b063ff5
Replace raw pointers as functions arguments in DirectoryLockImpl; r=dom-workers-and-storage-reviewers,sg
https://hg.mozilla.org/integration/autoland/rev/90b66b85cd0b
Use NotNull for DirectoryLockImpl's member variables; r=dom-workers-and-storage-reviewers,sg
https://hg.mozilla.org/integration/autoland/rev/fe3c9d352124
Use InitializedOnce for DirectoryLockImpl::mOpenListener; r=dom-workers-and-storage-reviewers,janv,sg
https://hg.mozilla.org/integration/autoland/rev/298bb04b486a
Use FlippedOnce for DirectoryLockImpl::mInvalidate; r=dom-workers-and-storage-reviewers,sg
https://hg.mozilla.org/integration/autoland/rev/68162c5c5c05
Reduce raw pointers as functions arguments for DirectoryLock related functions in QuotaManager; r=dom-workers-and-storage-reviewers,sg

The leave-open keyword is there and there is no activity for 6 months.
:tt, maybe it's time to close this bug?

Flags: needinfo?(ttung)

(In reply to Release mgmt bot [:sylvestre / :calixte / :marco for bugbug] from comment #27)

The leave-open keyword is there and there is no activity for 6 months.
:tt, maybe it's time to close this bug?

I still have a few unfinished patches but they need to be updated and I am not sure if I will get back to them soon. Therefore, I will just close this bug and file another one when I complete them. Thanks for the notice anyway!

Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Flags: needinfo?(ttung)
Keywords: leave-open
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.