1619965 - Remove duplication between DeleteDatabaseOp::NoteDatabaseClosed and OpenDatabaseOp::NoteDatabaseClosed

Status: RESOLVED FIXED

(Core :: Storage: IndexedDB, task, P2)

Comment 1

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

Attachment: Bug 1619965 - Remove duplication between NoteDatabaseClosed methods. r=#dom-workers-and-storage,janv

Comment 2

[Jan Varga [:janv]]

We set this to null in multiple places and it seems there's no good reason for that. It's too complicated. I have an experimental patch that sets mWaitingFactoryOp to nullptr only in FactoryOp::NoteDatabaseClosed, but I have to do a bit more testing.

Ok, so this all started in the original PBackground based IndexedDB implementation:
https://hg.mozilla.org/mozilla-central/rev/8892214038df

Then we had several fixes that had to deal with the fact that mWaitingFactoryOp is not immediately cleared in NoteDatabaseClose:
https://hg.mozilla.org/mozilla-central/rev/a9e5be3e74b2
https://hg.mozilla.org/mozilla-central/rev/236004185e7f
https://hg.mozilla.org/mozilla-central/rev/665f1887a112

I'm going to send a patch which simplifies these things.

Comment 3

[Jan Varga [:janv]]

Attachment: Bug 1619965 - Simplify clearing of mWaitingFactoryOp; r=sg,#dom-workers-and-storage-reviewers

mWaitingFactoryOp is now always cleared when the last maybe blocked database is
removed in FactoryOp::NoteDatabaseClosed.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/da983a0216c8
Remove duplication between NoteDatabaseClosed methods. r=janv,dom-workers-and-storage-reviewers
https://hg.mozilla.org/integration/autoland/rev/efa2dcf4f69e
Simplify clearing of mWaitingFactoryOp; r=sg,dom-workers-and-storage-reviewers

Comment 5

[Bogdan Tara[:bogdan_tara | bogdant]]

Backed out 2 changesets (bug 1619965) for bustages complaining about ActorsParent.cpp

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&selectedJob=296912091&fromchange=efa2dcf4f69e66ff94982776fb5428885ee4db5b&tochange=33d2485721c6ffd20a952d698754041a14be11b3&searchStr=build

Backout link: https://hg.mozilla.org/integration/autoland/rev/33d2485721c6ffd20a952d698754041a14be11b3

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=296912091&repo=autoland&lineNumber=44478

[task 2020-04-09T08:29:06.510Z] 08:29:06     INFO -  make[4]: Entering directory '/builds/worker/workspace/obj-build/dom/indexedDB'
[task 2020-04-09T08:29:06.518Z] 08:29:06     INFO -  /builds/worker/fetches/sccache/sccache /builds/worker/fetches/clang/bin/clang++ -std=gnu++17 -o ActorsParent.o -c  -I/builds/worker/workspace/obj-build/dist/stl_wrappers -I/builds/worker/workspace/obj-build/dist/system_wrappers -include /builds/worker/checkouts/gecko/config/gcc_hidden.h -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -fstack-protector-strong -DNDEBUG=1 -DTRIMMED=1 -DOS_POSIX=1 -DOS_LINUX=1 -DMOZ_HAS_MOZGLUE -DMOZILLA_INTERNAL_API -DIMPL_LIBXUL -DSTATIC_EXPORTABLE_JS_API -I/builds/worker/checkouts/gecko/dom/indexedDB -I/builds/worker/workspace/obj-build/dom/indexedDB -I/builds/worker/workspace/obj-build/ipc/ipdl/_ipdlheaders -I/builds/worker/checkouts/gecko/ipc/chromium/src -I/builds/worker/checkouts/gecko/ipc/glue -I/builds/worker/checkouts/gecko/dom/base -I/builds/worker/checkouts/gecko/dom/storage -I/builds/worker/checkouts/gecko/ipc/glue -I/builds/worker/checkouts/gecko/third_party/sqlite3/src -I/builds/worker/checkouts/gecko/xpcom/build -I/builds/worker/workspace/obj-build/dist/include -I/builds/worker/workspace/obj-build/dist/include/nspr -I/builds/worker/workspace/obj-build/dist/include/nss -fPIC -DMOZILLA_CLIENT -include /builds/worker/workspace/obj-build/mozilla-config.h -Qunused-arguments -Qunused-arguments -Wall -Wbitfield-enum-conversion -Wempty-body -Wignored-qualifiers -Woverloaded-virtual -Wpointer-arith -Wshadow-field-in-constructor-modified -Wsign-compare -Wtype-limits -Wunreachable-code -Wunreachable-code-return -Wwrite-strings -Wno-invalid-offsetof -Wclass-varargs -Wempty-init-stmt -Wfloat-overflow-conversion -Wfloat-zero-conversion -Wloop-analysis -Wc++2a-compat -Wcomma -Wimplicit-fallthrough -Wunused-function -Wunused-variable -Werror=non-literal-null-conversion -Wstring-conversion -Wtautological-overlap-compare -Wtautological-unsigned-enum-zero-compare -Wtautological-unsigned-zero-compare -Wno-error=tautological-type-limit-compare -Wno-inline-new-delete -Wno-error=deprecated-declarations -Wno-error=array-bounds -Wno-error=backend-plugin -Wno-error=return-std-move -Wno-error=atomic-alignment -Wformat -Wformat-security -Wno-gnu-zero-variadic-macro-arguments -Wno-unknown-warning-option -D_GLIBCXX_USE_CXX11_ABI=0 -fno-sized-deallocation -fno-aligned-new -fcrash-diagnostics-dir=/builds/worker/artifacts -fno-exceptions -fno-strict-aliasing -fno-rtti -ffunction-sections -fdata-sections -fno-exceptions -fno-math-errno -pthread -pipe -g -Xclang -load -Xclang /builds/worker/workspace/obj-build/build/clang-plugin/libclang-plugin.so -Xclang -add-plugin -Xclang moz-check -O2 -fno-omit-frame-pointer -funwind-tables -Werror -Wno-error=shadow -fexperimental-new-pass-manager  -MD -MP -MF .deps/ActorsParent.o.pp   /builds/worker/checkouts/gecko/dom/indexedDB/ActorsParent.cpp
[task 2020-04-09T08:29:06.518Z] 08:29:06    ERROR -  /builds/worker/checkouts/gecko/dom/indexedDB/ActorsParent.cpp:20571:3: error: Unused "kungFuDeathGrip" 'const RefPtr<mozilla::dom::indexedDB::(anonymous namespace)::FactoryOp>' objects constructed from temporary values are prohibited
[task 2020-04-09T08:29:06.518Z] 08:29:06     INFO -    const RefPtr<FactoryOp> waitingFactoryOp = std::move(info->mWaitingFactoryOp);
[task 2020-04-09T08:29:06.518Z] 08:29:06     INFO -    ^
[task 2020-04-09T08:29:06.518Z] 08:29:06     INFO -  /builds/worker/checkouts/gecko/dom/indexedDB/ActorsParent.cpp:20571:46: note: Please switch all accesses to this value to go through 'waitingFactoryOp', or explicitly pass 'waitingFactoryOp' to `mozilla::Unused`
[task 2020-04-09T08:29:06.518Z] 08:29:06     INFO -    const RefPtr<FactoryOp> waitingFactoryOp = std::move(info->mWaitingFactoryOp);
[task 2020-04-09T08:29:06.518Z] 08:29:06     INFO -                                               ^
[task 2020-04-09T08:29:06.518Z] 08:29:06     INFO -  1 error generated.
[task 2020-04-09T08:29:06.519Z] 08:29:06     INFO -  /builds/worker/checkouts/gecko/config/rules.mk:750: recipe for target 'ActorsParent.o' failed
[task 2020-04-09T08:29:06.519Z] 08:29:06    ERROR -  make[4]: *** [ActorsParent.o] Error 1
[task 2020-04-09T08:29:06.519Z] 08:29:06     INFO -  make[4]: Leaving directory '/builds/worker/workspace/obj-build/dom/indexedDB'
[task 2020-04-09T08:29:06.519Z] 08:29:06     INFO -  /builds/worker/checkouts/gecko/config/recurse.mk:74: recipe for target 'dom/indexedDB/target-objects' failed
[task 2020-04-09T08:29:06.519Z] 08:29:06    ERROR -  make[3]: *** [dom/indexedDB/target-objects] Error 2
[task 2020-04-09T08:29:06.519Z] 08:29:06     INFO -  make[3]: *** Waiting for unfinished jobs....

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/08f62d14f39f
Remove duplication between NoteDatabaseClosed methods. r=janv,dom-workers-and-storage-reviewers
https://hg.mozilla.org/integration/autoland/rev/0afcaad12487
Simplify clearing of mWaitingFactoryOp; r=sg,dom-workers-and-storage-reviewers

Comment 7

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/08f62d14f39f
https://hg.mozilla.org/mozilla-central/rev/0afcaad12487

Comment 8

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

Clearing NI, succesfully re-landed.

Comment 9

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

Comment on attachment 9130788 [details]
Bug 1619965 - Remove duplication between NoteDatabaseClosed methods. r=#dom-workers-and-storage,janv

Beta/Release Uplift Approval Request

• User impact if declined: Exposure to more complex control flow, which makes the analysis of resulting issues harder.

Please contact me for more details if necessary.

• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Simplification of control flow.
• String changes made/needed:

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9130788 [details]
Bug 1619965 - Remove duplication between NoteDatabaseClosed methods. r=#dom-workers-and-storage,janv

Simplifies some control flow in some complex code. Approved for 76.0b6.

Comment 11

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/releases/mozilla-beta/rev/b2a3237363ec
https://hg.mozilla.org/releases/mozilla-beta/rev/74befab3075a

Comment 12

[Narcis Beleuzu [:NarcisB]]

Backed out for build bustages on ActorsParent.cpp

Backout link: https://hg.mozilla.org/releases/mozilla-beta/rev/ae4f8f7008e9d990179ec9388d69422482ebb6b8
Log link: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=298256900&repo=mozilla-beta&lineNumber=30237

Comment 13

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

(In reply to Narcis Beleuzu [:NarcisB] from comment #12)

Backed out for build bustages on ActorsParent.cpp

Backout link: https://hg.mozilla.org/releases/mozilla-beta/rev/ae4f8f7008e9d990179ec9388d69422482ebb6b8
Log link: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=298256900&repo=mozilla-beta&lineNumber=30237

Ah, either the patch from Bug 1605075 must also be uplifted, or we need to modify this patch to work around the analyeer issue. Since Bug 1605075 is not about the production code, I would suggest the former.

Comment 14

[Andreea Pavel [:apavel]]

Can the tracking flag be changed until there is an uplift resolution here, so this doesn't show in the uplift query?

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Should be set to re-land for 76.0b7 now.

Comment 16

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/releases/mozilla-beta/rev/5cb97fba56ba
https://hg.mozilla.org/releases/mozilla-beta/rev/753f1893a512

Comment 17

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

Attachment: Bug 1619965 - Remove duplication between NoteDatabaseClosed methods. r=janv

Uplifted to esr68.

Comment 18

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

Attachment: Bug 1619965 - Simplify clearing of mWaitingFactoryOp. r=janv

mWaitingFactoryOp is now always cleared when the last maybe blocked database is
removed in FactoryOp::NoteDatabaseClosed.

Uplifted to esr68.

Depends on D74020

Comment 19

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

Comment on attachment 9146111 [details]
Bug 1619965 - Remove duplication between NoteDatabaseClosed methods. r=janv

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: As discussed.
• User impact if declined: Exposure to seemingly random crashes in contexts using IndexedDB.
• Fix Landed on Version: 76
• Risk to taking this patch: Medium
• Why is the change risky/not risky? (and alternatives if risky): The patch required some rebasing, due to several refactorings applied to the affected code since esr68.
• String or UUID changes made by this patch:

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9146111 [details]
Bug 1619965 - Remove duplication between NoteDatabaseClosed methods. r=janv

Fixes some IndexedDB crashes. Approved for 68.9esr.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr68/rev/d1c6d434e35e
https://hg.mozilla.org/releases/mozilla-esr68/rev/6e6e998a21de