Closed Bug 1666219 Opened 4 years ago Closed 4 years ago

Introduce QM_TRY_INSPECT macro and quota client specific equivalents

Categories

(Core :: Storage: Quota Manager, task)

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

Tracking

()

Status:
RESOLVED FIXED
Milestone:
83 Branch
Tracking Flags:
Tracking Status
firefox83 --- fixed

People

(Reporter: sg, Assigned: sg)

Details

Attachments

(2 files)

Bug 1666219 - Introduce QM_TRY_INSPECT to avoid unwrapping Result objects where not necessary.
47 bytes, text/x-phabricator-request
Details | Review
Bug 1666219 - Renamed QM_TRY_VAR etc. to QM_TRY_UNWRAP etc. r=#dom-workers-and-storage
47 bytes, text/x-phabricator-request
Details | Review
No description provided.

Ok, so currently we have these macros:

  1. tryResult is discarded
    QM_TRY, QM_DEBUG_TRY and QM_ASSERT_TRY

  2. target = tryResult.unwrap()
    QM_TRY_VAR, QM_DEBUG_TRY_VAR and QM_ASSERT_TRY_VAR

  3. return tryResult.unwrap()
    QM_TRY_RETURN, QM_DEBUG_TRY_RETURN and QM_ASSERT_TRY_RETURN

  4. return retVal (mozilla::Result is not involved)
    QM_FAIL and QM_DEBUG_FAIL

So, when mozilla::Result is involved the macro starts with QM_TRY
If tryResult is just discarded there's no additional suffix which makes sense.
If the success result is assigned to target then there's the _VAR suffix (I think _ASSIGN would make more sense if we didn't need a new macro for the inspect case).
If the success result is returned then there's the _RETURN suffix (again, the wording makes sense).

I somehow don't like that QM_TRY_VAR_INSPECT consists of 4 parts. I think we should rename QM_TRY_VAR to QM_TRY_UNWRAP and introduce QM_TRY_INSPECT.

I assume there's no use case for QM_TRY_RETURN to return tryresult.inspect()

Simon, WDYT ?

Flags: needinfo?(sgiesecke)

(In reply to Jan Varga [:janv] from comment #2)

I somehow don't like that QM_TRY_VAR_INSPECT consists of 4 parts. I think we should rename QM_TRY_VAR to QM_TRY_UNWRAP and introduce QM_TRY_INSPECT.

Yes, I like that!

I am not sure if we need QM_DEBUG_TRY_INSPECT and QM_ASSERT_TRY_INSPECT, or if we can just do the renaming to QM_DEBUG_TRY_UNWRAP and QM_ASSERT_TRY_UNWRAP for consistency.

I assume there's no use case for QM_TRY_RETURN to return tryresult.inspect()

I can't really imagine a use case for that either. A type where copying is cheaper than moving? Doesn't sound very likely. We shouldn't bother to add that :)

Flags: needinfo?(sgiesecke)
Summary: Introduce QM_TRY_VAR_INSPECT macro and quota client specific equivalents → Introduce QM_TRY_INSPECT macro and quota client specific equivalents

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

(In reply to Jan Varga [:janv] from comment #2)

I somehow don't like that QM_TRY_VAR_INSPECT consists of 4 parts. I think we should rename QM_TRY_VAR to QM_TRY_UNWRAP and introduce QM_TRY_INSPECT.

Yes, I like that!

Perfect!

I am not sure if we need QM_DEBUG_TRY_INSPECT and QM_ASSERT_TRY_INSPECT, or if we can just do the renaming to QM_DEBUG_TRY_UNWRAP and QM_ASSERT_TRY_UNWRAP for consistency.

Depends who will land first :)
I think you can land first, and I'll update my patch for the assert case after that (I'll see if I add debug and assert variations for the new inspect macto).
So for now, just do the renaming in this bug.

I assume there's no use case for QM_TRY_RETURN to return tryresult.inspect()

I can't really imagine a use case for that either. A type where copying is cheaper than moving? Doesn't sound very likely. We shouldn't bother to add that :)

Yes, I just asked to be 100% sure, so we can keep the naming scheme sane enough.

Attachment #9176849 - Attachment description: Bug 1666219 - Introduce QM_TRY_VAR_INSPECT to avoid unwrapping Result objects where not necessary. → Bug 1666219 - Introduce QM_TRY_INSPECT to avoid unwrapping Result objects where not necessary.
Pushed by sgiesecke@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2c37dc3918d3
Introduce QM_TRY_INSPECT to avoid unwrapping Result objects where not necessary. r=dom-workers-and-storage-reviewers,janv

https://hg.mozilla.org/mozilla-central/rev/2c37dc3918d3

Status: ASSIGNED → RESOLVED
Closed: 4 years ago
status-firefox83: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 83 Branch
Pushed by sgiesecke@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5ef18af381ec
Renamed QM_TRY_VAR etc. to QM_TRY_UNWRAP etc. r=dom-workers-and-storage-reviewers,ttung,janv

Backed out 5 changesets (bug 1666219, bug 1665347) for indexedDB related bustage.

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&searchStr=build&fromchange=70158c37683b814f926479974cd87a5a8a41e3cd&tochange=9ec809746446d612146b14372f9f63d90f0edea2&selectedTaskRun=TOQjs4b9Tk2ZGUIYCjsAJg.0

Backout link: https://hg.mozilla.org/integration/autoland/rev/9ec809746446d612146b14372f9f63d90f0edea2

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

[task 2020-10-01T14:29:15.073Z] 14:29:15     INFO -  mozmake.EXE[4]: Entering directory 'z:/build/workspace/obj-build/dom/indexedDB'
[task 2020-10-01T14:29:15.073Z] 14:29:15     INFO -  z:/build/fetches/sccache/sccache.exe z:/build/fetches/clang/bin/clang.exe --driver-mode=cl -Xclang -std=c++17 -FoActorsParent.obj -c  -Iz:/build/workspace/obj-build/dist/stl_wrappers -guard:cf -U_FORTIFY_SOURCE -Xclang -fno-common -DNDEBUG=1 -DTRIMMED=1 -DUNICODE -D_UNICODE -D_CRT_RAND_S -DCERT_CHAIN_PARA_HAS_EXTRA_FIELDS -D_SECURE_ATL -DCHROMIUM_BUILD -DU_STATIC_IMPLEMENTATION -DOS_WIN=1 -DWIN32 -D_WIN32 -D_WINDOWS -DWIN32_LEAN_AND_MEAN -DCOMPILER_MSVC -DWINAPI_NO_BUNDLED_LIBRARIES -DMOZ_HAS_MOZGLUE -DMOZILLA_INTERNAL_API -DIMPL_LIBXUL -DSTATIC_EXPORTABLE_JS_API -Iz:/build/build/src/dom/indexedDB -Iz:/build/workspace/obj-build/dom/indexedDB -Iz:/build/workspace/obj-build/ipc/ipdl/_ipdlheaders -Iz:/build/build/src/ipc/chromium/src -Iz:/build/build/src/ipc/glue -Iz:/build/build/src/dom/base -Iz:/build/build/src/dom/storage -Iz:/build/build/src/ipc/glue -Iz:/build/build/src/third_party/sqlite3/src -Iz:/build/build/src/xpcom/build -Iz:/build/workspace/obj-build/dist/include -Iz:/build/workspace/obj-build/dist/include/nspr -Iz:/build/workspace/obj-build/dist/include/nss -MD -FI z:/build/workspace/obj-build/mozilla-config.h -DMOZILLA_CLIENT -Qunused-arguments -Qunused-arguments -fsanitize=address -fsanitize-blacklist=z:/build/build/src/build/sanitizers/asan_blacklist_win.txt -fcrash-diagnostics-dir=z:/build/public/build -fcrash-diagnostics-dir=/z/build/public/build -fcrash-diagnostics-dir=/z/build/public/build -TP -Zc:sizedDealloc- -D_HAS_EXCEPTIONS=0 -W3 -Gy -Zc:inline -Wno-inline-new-delete -Wno-invalid-offsetof -Wno-microsoft-enum-value -Wno-microsoft-include -Wno-unknown-pragmas -Wno-ignored-pragmas -Wno-deprecated-declarations -Wno-invalid-noreturn -Wno-inconsistent-missing-override -Wno-implicit-exception-spec-mismatch -Wno-microsoft-exception-spec -Wno-unused-local-typedef -Wno-ignored-attributes -Wno-used-but-marked-unused -D_SILENCE_TR1_NAMESPACE_DEPRECATION_WARNING -GR- -Z7 -Xclang -load -Xclang z:/build/workspace/obj-build/build/clang-plugin/clang-plugin.dll -Xclang -add-plugin -Xclang moz-check -O2 -gline-tables-only -Oy- -Werror -Xclang -fexperimental-new-pass-manager  -Xclang -MP -Xclang -dependency-file -Xclang .deps/ActorsParent.obj.pp -Xclang -MT -Xclang ActorsParent.obj   z:/build/build/src/dom/indexedDB/ActorsParent.cpp
[task 2020-10-01T14:29:15.073Z] 14:29:15     INFO -  z:/build/build/src/dom/indexedDB/ActorsParent.cpp(692,42): error: unused variable 'hasResult' [-Werror,-Wunused-variable]
[task 2020-10-01T14:29:15.073Z] 14:29:15     INFO -    IDB_TRY_INSPECT(const DebugOnly<bool>& hasResult,
[task 2020-10-01T14:29:15.073Z] 14:29:15     INFO -                                           ^
[task 2020-10-01T14:29:15.073Z] 14:29:15     INFO -  z:/build/build/src/dom/indexedDB/ActorsParent.cpp(7555,42): error: unused variable 'hasResult' [-Werror,-Wunused-variable]
[task 2020-10-01T14:29:15.074Z] 14:29:15     INFO -    IDB_TRY_INSPECT(const DebugOnly<bool>& hasResult,
[task 2020-10-01T14:29:15.074Z] 14:29:15     INFO -                                           ^
[task 2020-10-01T14:29:15.074Z] 14:29:15     INFO -  2 errors generated.
[task 2020-10-01T14:29:15.074Z] 14:29:15     INFO -  z:/build/build/src/config/rules.mk:723: recipe for target 'ActorsParent.obj' failed
[task 2020-10-01T14:29:15.074Z] 14:29:15     INFO -  mozmake.EXE[4]: *** [ActorsParent.obj] Error 1
[task 2020-10-01T14:29:15.074Z] 14:29:15     INFO -  mozmake.EXE[4]: Leaving directory 'z:/build/workspace/obj-build/dom/indexedDB'
[task 2020-10-01T14:29:15.074Z] 14:29:15     INFO -  mozmake.EXE[4]: *** Waiting for unfinished jobs....
Flags: needinfo?(sgiesecke)
Pushed by sgiesecke@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7b36a79247b7
Renamed QM_TRY_VAR etc. to QM_TRY_UNWRAP etc. r=dom-workers-and-storage-reviewers,ttung,janv

Backed out for crashing hazard task:

https://hg.mozilla.org/integration/autoland/rev/5c1c0137dbd12e79fdb643817465a9be666ea3f0

Log of crashing hazard run: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=317322047&repo=autoland&lineNumber=14116-14191

[task 2020-10-01T21:18:13.937Z] 12:50.70 In file included from /builds/worker/checkouts/gecko/obj-analyzed/dist/include/mozilla/dom/indexedDB/Key.h:10,
[task 2020-10-01T21:18:13.938Z] 12:50.70 from /builds/worker/checkouts/gecko/obj-analyzed/dist/include/mozilla/dom/IDBCursorType.h:11,
[task 2020-10-01T21:18:13.938Z] 12:50.71 from /builds/worker/checkouts/gecko/dom/indexedDB/ActorsChild.h:12,
[task 2020-10-01T21:18:13.938Z] 12:50.71 from /builds/worker/checkouts/gecko/dom/indexedDB/ActorsChild.cpp:7,
[task 2020-10-01T21:18:13.938Z] 12:50.71 from Unified_cpp_dom_indexedDB0.cpp:2:
[task 2020-10-01T21:18:13.938Z] 12:50.71 /builds/worker/checkouts/gecko/obj-analyzed/dist/include/mozilla/dom/indexedDB/IDBResult.h: In instantiation of 'mozilla::ErrorResult mozilla::dom::indexedDB::detail::IDBError<S>::ExtractErrorResult(SpecialValueMappers ...) [with SpecialValueMappers = {nsresult ()(const std::integral_constant<mozilla::dom::indexedDB::IDBSpecialValue, mozilla::dom::indexedDB::IDBSpecialValue::Invalid>&)}; mozilla::dom::indexedDB::IDBSpecialValue ...S = {mozilla::dom::indexedDB::IDBSpecialValue::Invalid}]':
[task 2020-10-01T21:18:13.938Z] 12:50.71 /builds/worker/checkouts/gecko/dom/indexedDB/IDBFactory.cpp:451:55: required from here
[task 2020-10-01T21:18:13.938Z] 12:50.71 /builds/worker/checkouts/gecko/obj-analyzed/dist/include/mozilla/dom/indexedDB/IDBResult.h:109:13: internal compiler error: Segmentation fault
[task 2020-10-01T21:18:13.938Z] 12:50.71 109 | }...);
[task 2020-10-01T21:18:13.938Z] 12:50.71 | ^
[task 2020-10-01T21:18:13.941Z] 12:50.71 0xbc8e8f crash_signal
[task 2020-10-01T21:18:13.941Z] 12:50.71 ../../gcc-source/gcc/toplev.c:326
[task 2020-10-01T21:18:13.941Z] 12:50.71 0x719a50 argument_pack_element_is_expansion_p
[task 2020-10-01T21:18:13.941Z] 12:50.71 ../../gcc-source/gcc/cp/pt.c:11600
[task 2020-10-01T21:18:13.941Z] 12:50.71 0x73ab08 tsubst_pack_expansion(tree_node, tree_node*, int, tree_node*)
[task 2020-10-01T21:18:13.941Z] 12:50.71 ../../gcc-source/gcc/cp/pt.c:12250
[task 2020-10-01T21:18:13.941Z] 12:50.71 0x72d3c5 tsubst_copy_and_build(tree_node*, tree_node*, int, tree_node*, bool, bool)
[task 2020-10-01T21:18:13.941Z] 12:50.71 ../../gcc-source/gcc/cp/pt.c:18833
[task 2020-10-01T21:18:13.941Z] 12:50.71 0x735144 tsubst_copy_and_build(tree_node*, tree_node*, int, tree_node*, bool, bool)
[task 2020-10-01T21:18:13.941Z] 12:50.71 ../../gcc-source/gcc/cp/pt.c:18241
[task 2020-10-01T21:18:13.941Z] 12:50.71 0x735144 tsubst_expr(tree_node*, tree_node*, int, tree_node*, bool)
[task 2020-10-01T21:18:13.941Z] 12:50.71 ../../gcc-source/gcc/cp/pt.c:17917
[task 2020-10-01T21:18:13.941Z] 12:50.71 0x7347de tsubst_expr(tree_node*, tree_node*, int, tree_node*, bool)
[task 2020-10-01T21:18:13.941Z] 12:50.71 ../../gcc-source/gcc/cp/pt.c:17033
[task 2020-10-01T21:18:13.941Z] 12:50.71 0x734960 tsubst_expr(tree_node*, tree_node*, int, tree_node*, bool)
[task 2020-10-01T21:18:13.941Z] 12:50.71 ../../gcc-source/gcc/cp/pt.c:17023
[task 2020-10-01T21:18:13.941Z] 12:50.71 0x734ad1 tsubst_expr(tree_node*, tree_node*, int, tree_node*, bool)
[task 2020-10-01T21:18:13.941Z] 12:50.71 ../../gcc-source/gcc/cp/pt.c:17324
[task 2020-10-01T21:18:13.942Z] 12:50.71 0x733d0d instantiate_decl(tree_node*, bool, bool)
[task 2020-10-01T21:18:13.942Z] 12:50.71 ../../gcc-source/gcc/cp/pt.c:24773
[task 2020-10-01T21:18:13.942Z] 12:50.71 0x749303 instantiate_pending_templates(int)
[task 2020-10-01T21:18:13.942Z] 12:50.71 ../../gcc-source/gcc/cp/pt.c:24889
[task 2020-10-01T21:18:13.942Z] 12:50.71 0x6a3668 c_parse_final_cleanups()
[task 2020-10-01T21:18:13.942Z] 12:50.71 ../../gcc-source/gcc/cp/decl2.c:4818
[task 2020-10-01T21:18:13.942Z] 12:50.71 Please submit a full bug report,
[task 2020-10-01T21:18:13.942Z] 12:50.71 with preprocessed source if appropriate.
[task 2020-10-01T21:18:13.942Z] 12:50.71 Please include the complete backtrace with any bug report.
[task 2020-10-01T21:18:13.942Z] 12:50.71 See https://gcc.gnu.org/bugs/ for instructions.

Pushed by sgiesecke@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/88ea67980c3a
Renamed QM_TRY_VAR etc. to QM_TRY_UNWRAP etc. r=dom-workers-and-storage-reviewers,ttung,janv

Re-landed successfully now. The problem was in patches for other bugs that landed together with this one.

Flags: needinfo?(sgiesecke)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: