1641231 - Follow pointer guideline for dom/quota/ActorsParent.cpp

Status: RESOLVED FIXED

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

Comment 1

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1641231 - Replace new operator with MakeRefPtr for constructing actor classes;

Comment 2

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1641231 - Reduce the usage for passing raw pointers in RegisterNormalOriginOp/UnregisterNormalOriginOp;

Depends on D77077

Comment 3

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1641231 - Do not pass a raw pointer only for dereferencing for functions in actor classes;

Depends on D77078

Comment 4

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1641231 - Use Result instead of an out parameter for QuotaUsageRequestBase::GetUsageForOrigin;

Depends on D77079

Comment 5

[Tom Tung [:tt, :ttung]]

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

Comment 6

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1641231 - Split GetUsageForOrigin into a smaller function;

Comment 7

[Tom Tung [:tt, :ttung]]

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

Comment 8

[Pulsebot]

Pushed by [e-mail]:
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

Comment 9

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/9466bba20117
https://hg.mozilla.org/mozilla-central/rev/7106175afc6a
https://hg.mozilla.org/mozilla-central/rev/f0cdc44b0023
https://hg.mozilla.org/mozilla-central/rev/cf429262e727
https://hg.mozilla.org/mozilla-central/rev/738524c19b7f

Comment 10

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1641231 - Replace raw pointers as functions arguments in DirectoryLockImpl;

Comment 11

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1641231 - Use NotNull for DirectoryLockImpl's member variables;

Depends on D78062

Comment 12

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1641231 - Use InitializedOnce for DirectoryLockImpl::mOpenListener;

Depends on D78065

Comment 13

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1641231 - Use FlippedOnce for DirectoryLockImpl::mInvalidate;

Depends on D78066

Comment 14

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1641231 - Reduce raw pointers as functions arguments for DirectoryLock related functions in QuotaManager;

Depends on D78067

Comment 15

[Tom Tung [:tt, :ttung]]

I plan to make DirectoryLock as a SafeRefPtr and update DirectoryLockAcquired

Comment 16

[Pulsebot]

Pushed by [e-mail]:
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

Comment 17

[Natalia Csoregi [:nataliaCs]]

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

Comment 18

[Tom Tung [:tt, :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.

Comment 19

[Tom Tung [:tt, :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.

Comment 20

[Tom Tung [:tt, :ttung]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=aa60a3402e72f7ffbb3de46d457de776c775e974

Comment 21

[Simon Giesecke [:sg] [he/him]]

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

Comment 22

[Tom Tung [:tt, :ttung]]

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

Comment 23

[Tom Tung [:tt, :ttung]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6425ae559da0b55f42cab7db02c9a9d95e4df9d

Comment 24

[Tom Tung [:tt, :ttung]]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6425ae559da0b55f42cab7db02c9a9d95e4df9d&selectedTaskRun=cucOVaNWR661WNlR3q6C6A-0

I don't see anything suspecious, so I am going to land these patches again.

Comment 25

[Pulsebot]

Pushed by [e-mail]:
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

Comment 26

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/62076b063ff5
https://hg.mozilla.org/mozilla-central/rev/90b66b85cd0b
https://hg.mozilla.org/mozilla-central/rev/fe3c9d352124
https://hg.mozilla.org/mozilla-central/rev/298bb04b486a
https://hg.mozilla.org/mozilla-central/rev/68162c5c5c05

Comment 27

[BugBot [:suhaib / :marco/ :calixte]]

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

Comment 28

[Tom Tung [:tt, :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!