Closed Bug 1665347 Opened 7 months ago Closed 6 months ago

Reduce overlap between IDBResult and mozilla::Result

Categories

(Core :: Storage: IndexedDB, task)

task

Tracking

()

RESOLVED FIXED
83 Branch
Tracking Status
firefox83 --- fixed

People

(Reporter: sg, Assigned: sg)

References

Details

Attachments

(4 files)

There is some conceptual overlap between IDBResult and mozilla::Result. Since the implementation of IDBResult is non-trivial and the APIs differ slightly and make integration with IDB_TRY hard, it would be desirable to somehow merge them, and replace IDBResult<V, ...> by something like mozilla::Result<V, IDBError<...>>.

That would be great.

Assignee: nobody → sgiesecke
Depends on: 1644379
Depends on: 1666307
Pushed by sgiesecke@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3b81349a5f39
Make IDBResult based on mozilla::Result. r=dom-workers-and-storage-reviewers,janv
https://hg.mozilla.org/integration/autoland/rev/64a086636738
Use IDB_TRY with IDBResult. r=dom-workers-and-storage-reviewers,janv
https://hg.mozilla.org/integration/autoland/rev/9159fdd05097
Remove namespace qualification on mozilla::Ok. r=dom-workers-and-storage-reviewers,janv
https://hg.mozilla.org/integration/autoland/rev/5cd7e2d67f91
Do not indicate SpecialValues::Invalid could be returned on functions that never return it. r=dom-workers-and-storage-reviewers,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/af8025162523
Make IDBResult based on mozilla::Result. r=dom-workers-and-storage-reviewers,janv
https://hg.mozilla.org/integration/autoland/rev/435f02d9f1d7
Use IDB_TRY with IDBResult. r=dom-workers-and-storage-reviewers,janv
https://hg.mozilla.org/integration/autoland/rev/212c9df41410
Remove namespace qualification on mozilla::Ok. r=dom-workers-and-storage-reviewers,janv
https://hg.mozilla.org/integration/autoland/rev/c261ec06a826
Do not indicate SpecialValues::Invalid could be returned on functions that never return it. r=dom-workers-and-storage-reviewers,janv

Backed outfor 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.

Steve, could you give comment #9 a look?

Flags: needinfo?(sgiesecke) → needinfo?(sphink)
See Also: → 1668766

(I'm not ignoring this, though I'm having trouble reproducing this locally.)

(In reply to Steve Fink [:sfink] [:s:] from comment #11)

(I'm not ignoring this, though I'm having trouble reproducing this locally.)

Thanks for reporting back! I tried to adapt the #ifdef condition to explicitly use the second version when XGILL_PLUGIN is defined and am waiting for a try job to see if that fixes the issue: https://treeherder.mozilla.org/#/jobs?repo=try&revision=72dd090e907549c5892cea3edaee560f76bd1d90&selectedTaskRun=RwaCSF00TnCKxBXQwMW84Q.0

It seems that worked, so I'm clearing the NI for you.

Flags: needinfo?(sphink)
Pushed by sgiesecke@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/95db4cd7c8b1
Make IDBResult based on mozilla::Result. r=dom-workers-and-storage-reviewers,janv
https://hg.mozilla.org/integration/autoland/rev/d16c64549d44
Use IDB_TRY with IDBResult. r=dom-workers-and-storage-reviewers,janv
https://hg.mozilla.org/integration/autoland/rev/984eed4583de
Remove namespace qualification on mozilla::Ok. r=dom-workers-and-storage-reviewers,janv
https://hg.mozilla.org/integration/autoland/rev/8d71aed8e8c2
Do not indicate SpecialValues::Invalid could be returned on functions that never return it. r=dom-workers-and-storage-reviewers,janv

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

(In reply to Steve Fink [:sfink] [:s:] from comment #11)

(I'm not ignoring this, though I'm having trouble reproducing this locally.)

Thanks for reporting back! I tried to adapt the #ifdef condition to explicitly use the second version when XGILL_PLUGIN is defined and am waiting for a try job to see if that fixes the issue:

I never was able to reproduce this, though it seems like that it's a gcc problem unrelated to the hazard analysis, given the stack shown. (The analysis doesn't modify anything.)

I may end up upgrading the gcc as a result of another problem, so I might get lucky and fix this ICE accidentally? (I can always hope.)

My only concern was that this might hide something from the analysis, but I looked and saw how you did this -- thanks! That seems totally fine.

FYI - we have reported the gcc ICE and their conclusion is that Firefox code isn't valid C++ and needs a fix. Please see https://gcc.gnu.org/bugzilla/show_bug.cgi?id=97358 for all the details (the fix for Firefox, update to gcc to reject the invalid code instead of throwing an ICE).

Regressions: 1671345

(In reply to Dan Horák from comment #17)

FYI - we have reported the gcc ICE and their conclusion is that Firefox code isn't valid C++ and needs a fix. Please see https://gcc.gnu.org/bugzilla/show_bug.cgi?id=97358 for all the details (the fix for Firefox, update to gcc to reject the invalid code instead of throwing an ICE).

Thanks a lot, I filed Bug 1671345 to fix this. It seems then that there's also a bug in clang and it should actually reject the current code.

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

(In reply to Dan Horák from comment #17)

FYI - we have reported the gcc ICE and their conclusion is that Firefox code isn't valid C++ and needs a fix. Please see https://gcc.gnu.org/bugzilla/show_bug.cgi?id=97358 for all the details (the fix for Firefox, update to gcc to reject the invalid code instead of throwing an ICE).

Thanks a lot, I filed Bug 1671345 to fix this. It seems then that there's also a bug in clang and it should actually reject the current code.

I reported this as https://bugs.llvm.org/show_bug.cgi?id=47852 now.

You need to log in before you can comment on or make changes to this bug.