Closed Bug 1746090 Opened 3 years ago Closed 3 years ago

[hazards] Prevent false alarms when a refcounted variable goes out of scope when it can be proven to not Release

Categories

(Core :: JavaScript: GC, enhancement, P1)

Product:
Core
Component:
JavaScript: GC
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
98 Branch
Tracking Flags:
Tracking Status
firefox98 --- fixed

People

(Reporter: sfink, Assigned: sfink)

References

(Blocks 1 open bug)

Details

Attachments

(10 files)

Bug 1746090 - Generalize traversing through a function's CFG with BFS_upwards().
48 bytes, text/x-phabricator-request
Details | Review
Bug 1746090 - Move some CFG-traversing code from analyzeRoots.js to the common CFG.js, to prepare for calling it from other phases of the analysis.
48 bytes, text/x-phabricator-request
Details | Review
Bug 1746090 - Switch to command-line argument parser.
48 bytes, text/x-phabricator-request
Details | Review
Bug 1746090 - Generate gcEdges file with computeCallgraph instead of computeGCFunctions
48 bytes, text/x-phabricator-request
Details | Review
Bug 1746090 - Annotate edges that invoke destructors on ref-counted values that can be proven to not Release
48 bytes, text/x-phabricator-request
Details | Review
Bug 1746090 - Fix the false positive from a loop-scoped GC pointer named temporary
48 bytes, text/x-phabricator-request
Details | Review
Bug 1746090 - Switch RootList::init from taking an AutoCheckCannotGC token to returning one
48 bytes, text/x-phabricator-request
Details | Review
Bug 1746090 - Replace the overcomplicated bodyEatsVariable with a quick linear-only test, and improve the comments around std::move + UniquePtr&& handling
48 bytes, text/x-phabricator-request
Details | Review
Bug 1746090 - Fix return value handling, specifically when the final edge in a function body GCs and is treating as using the return value. (This fixes false alarms when returning nullptr in the scope of a GC'ing RAII destructor.)
48 bytes, text/x-phabricator-request
Details | Review
Bug 1746090 - arg0_holder can have a non *Argument type, eg if it's using SpiderMonkeyInterfaceRooter.
48 bytes, text/x-phabricator-request
Details | Review

The basic scenario:

JSObject* f() {
  Rooted<JSObject*> obj = gcpointer();
  RefPtr<Foo> r = blah();
  ...
  r.forget();
  return obj;
}

The hazard analysis currently rejects this, because the obj return value is extracted into a register, then RefPtr<Foo>::~RefPtr() is invoked. The hazard analysis thinks of that destructor as a GC call, which it in fact is, because if the refcount happens to go to zero during destruction, Release() will be called and there are a handful of Releases that can in fact GC.

But in this case, the refcount can be proven to be zero, so Release will not be called.

Blocks: hazard
Assignee: nobody → sphink
Status: NEW → ASSIGNED
Severity: -- → N/A
Priority: -- → P1

Ugh, looks like these changes revealed some false positives. The previous code looks like it wasn't quite correct in how it traversed the graph, and there are a few places where we depend on some Maybe<AutoCheckCannotGC> that aren't really correct. (As in, the code is fine, but the proof that it was safe was wrong.) It seems like it'll require some kind of annotations.

Pushed by sfink@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/ff509fe4671d Generalize traversing through a function's CFG with BFS_upwards(). r=jonco https://hg.mozilla.org/integration/autoland/rev/5c5742535301 Move some CFG-traversing code from analyzeRoots.js to the common CFG.js, to prepare for calling it from other phases of the analysis. r=jonco https://hg.mozilla.org/integration/autoland/rev/774bdb9939a9 Switch to command-line argument parser. r=jonco https://hg.mozilla.org/integration/autoland/rev/a9c16e721533 Generate gcEdges file with computeCallgraph instead of computeGCFunctions r=jonco https://hg.mozilla.org/integration/autoland/rev/54a60388fb11 Annotate edges that invoke destructors on ref-counted values that can be proven to not Release r=jonco https://hg.mozilla.org/integration/autoland/rev/1ed9d77885c6 Fix the false positive from a loop-scoped GC pointer named temporary r=jonco https://hg.mozilla.org/integration/autoland/rev/2b0caa13d0fc Switch RootList::init from taking an AutoCheckCannotGC token to returning one r=jonco https://hg.mozilla.org/integration/autoland/rev/c288fe1c6c84 Replace the overcomplicated bodyEatsVariable with a quick linear-only test, and improve the comments around std::move + UniquePtr&& handling r=jonco https://hg.mozilla.org/integration/autoland/rev/f4e4bf6ba8ff Fix return value handling, specifically when the final edge in a function body GCs and is treating as using the return value. (This fixes false alarms when returning nullptr in the scope of a GC'ing RAII destructor.) r=jonco https://hg.mozilla.org/integration/autoland/rev/edbf96722e4b arg0_holder can have a non *Argument type, eg if it's using SpiderMonkeyInterfaceRooter. r=jonco

Backed out 10 changesets (Bug 1746090) for causing build bustages on TestingFunctions.cpp.
Backout link
Push with failures
Failure Log

Flags: needinfo?(sphink)
Pushed by sfink@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/b9b4558c03dc Generalize traversing through a function's CFG with BFS_upwards(). r=jonco https://hg.mozilla.org/integration/autoland/rev/2f7a62b93266 Move some CFG-traversing code from analyzeRoots.js to the common CFG.js, to prepare for calling it from other phases of the analysis. r=jonco https://hg.mozilla.org/integration/autoland/rev/a0b1735ffffd Switch to command-line argument parser. r=jonco https://hg.mozilla.org/integration/autoland/rev/fe5ac63668b4 Generate gcEdges file with computeCallgraph instead of computeGCFunctions r=jonco https://hg.mozilla.org/integration/autoland/rev/3a282a6b5f65 Annotate edges that invoke destructors on ref-counted values that can be proven to not Release r=jonco https://hg.mozilla.org/integration/autoland/rev/92a55bc8c1c8 Fix the false positive from a loop-scoped GC pointer named temporary r=jonco https://hg.mozilla.org/integration/autoland/rev/86dfecd590e8 Switch RootList::init from taking an AutoCheckCannotGC token to returning one r=jonco https://hg.mozilla.org/integration/autoland/rev/42586bdfd56f Replace the overcomplicated bodyEatsVariable with a quick linear-only test, and improve the comments around std::move + UniquePtr&& handling r=jonco https://hg.mozilla.org/integration/autoland/rev/aea63ceb5ff5 Fix return value handling, specifically when the final edge in a function body GCs and is treating as using the return value. (This fixes false alarms when returning nullptr in the scope of a GC'ing RAII destructor.) r=jonco https://hg.mozilla.org/integration/autoland/rev/132c6be41ec2 arg0_holder can have a non *Argument type, eg if it's using SpiderMonkeyInterfaceRooter. r=jonco

Backed out 10 changesets (Bug 1746090) for causing failures in HeapSnapshot.cpp CLOSED TREE

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&searchStr=linux%2C18.04%2Cx64%2Cwebrender%2Cdebug%2Cxpcshell%2Ctests%2Ctest-linux1804-64-qr%2Fdebug-xpcshell-e10s%2Cx5&revision=132c6be41ec2551241fe3e759580ae58c1ca1dcc&selectedTaskRun=H6xI91PwQIOlDln4yhyCAw.0

Log: https://treeherder.mozilla.org/logviewer?job_id=364912510&repo=autoland

[task 2022-01-21T00:47:12.938Z] 00:47:12 WARNING - TEST-UNEXPECTED-FAIL | devtools/shared/heapsnapshot/tests/xpcshell/test_HeapAnalyses_takeCensus_04.js | xpcshell return code: -11
[task 2022-01-21T00:47:12.939Z] 00:47:12 INFO - TEST-INFO took 247ms
[task 2022-01-21T00:47:12.939Z] 00:47:12 INFO - >>>>>>>
[task 2022-01-21T00:47:12.939Z] 00:47:12 INFO - PID 12917 | [Parent 12917, Main Thread] WARNING: Couldn't get the user appdata directory. Crash events may not be produced.: file /builds/worker/checkouts/gecko/toolkit/crashreporter/nsExceptionHandler.cpp:2965
[task 2022-01-21T00:47:12.939Z] 00:47:12 INFO - (xpcshell/head.js) | test MAIN run_test pending (1)
[task 2022-01-21T00:47:12.940Z] 00:47:12 INFO - (xpcshell/head.js) | test run_next_test 0 pending (2)
[task 2022-01-21T00:47:12.940Z] 00:47:12 INFO - (xpcshell/head.js) | test MAIN run_test finished (2)
[task 2022-01-21T00:47:12.940Z] 00:47:12 INFO - running event loop
[task 2022-01-21T00:47:12.940Z] 00:47:12 INFO - devtools/shared/heapsnapshot/tests/xpcshell/test_HeapAnalyses_takeCensus_04.js | Starting test
[task 2022-01-21T00:47:12.942Z] 00:47:12 INFO - (xpcshell/head.js) | test test pending (2)
[task 2022-01-21T00:47:12.942Z] 00:47:12 INFO - PID 12917 | [Parent 12917, Main Thread] WARNING: NS_ENSURE_SUCCESS(rv, kKnownEsrVersion) failed with result 0x80004002 (NS_NOINTERFACE): file /builds/worker/checkouts/gecko/toolkit/components/resistfingerprinting/nsRFPService.cpp:560
[task 2022-01-21T00:47:12.942Z] 00:47:12 INFO - PID 12917 | Assertion failure: roots.initialized(), at /builds/worker/checkouts/gecko/devtools/shared/heapsnapshot/HeapSnapshot.cpp:786
[task 2022-01-21T00:47:12.942Z] 00:47:12 INFO - PID 12917 | #01: mozilla::dom::ChromeUtils::SaveHeapSnapshotShared(mozilla::dom::GlobalObject&, mozilla::dom::HeapSnapshotBoundaries const&, nsTSubstring<char16_t>&, nsTSubstring<char16_t>&, mozilla::ErrorResult&) [devtools/shared/heapsnapshot/HeapSnapshot.cpp:1483]
[task 2022-01-21T00:47:12.942Z] 00:47:12 INFO - PID 12917 | #02: mozilla::dom::ChromeUtils::SaveHeapSnapshot(mozilla::dom::GlobalObject&, mozilla::dom::HeapSnapshotBoundaries const&, nsTSubstring<char16_t>&, mozilla::ErrorResult&) [devtools/shared/heapsnapshot/HeapSnapshot.cpp:1529]
[task 2022-01-21T00:47:12.943Z] 00:47:12 INFO - PID 12917 | #03: mozilla::dom::ChromeUtils_Binding::saveHeapSnapshot(JSContext*, unsigned int, JS::Value*) [s3:gecko-generated-sources:57ee2e519b7ade52dd478a265547fabc43d590df97b30d43c92781f0f62d07901e7e44122a5fe995158849b11f04087fda5070c85db0037cb21478e956aea809/dom/bindings/ChromeUtilsBinding.cpp::3652]
[task 2022-01-21T00:47:12.944Z] 00:47:12 INFO - PID 12917 | #04: CallJSNative(JSContext*, bool ()(JSContext, unsigned int, JS::Value*), js::CallReason, JS::CallArgs const&) [js/src/vm/Interpreter.cpp:425]
[task 2022-01-21T00:47:12.945Z] 00:47:12 INFO - PID 12917 | #05: js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason) [js/src/vm/Interpreter.cpp:512]
[task 2022-01-21T00:47:12.945Z] 00:47:12 INFO - PID 12917 | #06: InternalCall(JSContext*, js::AnyInvokeArgs const&, js::CallReason) [js/src/vm/Interpreter.cpp:572]
[task 2022-01-21T00:47:12.946Z] 00:47:12 INFO - PID 12917 | #07: Interpret(JSContext*, js::RunState&) [js/src/vm/Interpreter.cpp:3309]
[task 2022-01-21T00:47:12.946Z] 00:47:12 INFO - PID 12917 | #08: js::RunScript(JSContext*, js::RunState&) [js/src/vm/Interpreter.cpp:394]
[task 2022-01-21T00:47:12.947Z] 00:47:12 INFO - PID 12917 | #09: js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason) [js/src/vm/Interpreter.cpp:544]
[task 2022-01-21T00:47:12.947Z] 00:47:12 INFO - PID 12917 | #10: InternalCall(JSContext*, js::AnyInvokeArgs const&, js::CallReason) [js/src/vm/Interpreter.cpp:572]
[task 2022-01-21T00:47:12.948Z] 00:47:12 INFO - PID 12917 | #11: js::jit::DoCallFallback(JSContext*, js::jit::BaselineFrame*, js::jit::ICFallbackStub*, unsigned int, JS::Value*, JS::MutableHandle<JS::Value>) [js/src/jit/BaselineIC.cpp:1595]
[task 2022-01-21T00:47:12.948Z] 00:47:12 INFO - PID 12917 | #12: ??? (???:???)
[task 2022-01-21T00:47:12.948Z] 00:47:12 INFO - PID 12917 | ExceptionHandler::GenerateDump cloned child 12930
[task 2022-01-21T00:47:12.949Z] 00:47:12 INFO - PID 12917 | ExceptionHandler::SendContinueSignalToChild sent continue signal to child
[task 2022-01-21T00:47:12.950Z] 00:47:12 INFO - PID 12917 | ExceptionHandler::WaitForContinueSignal waiting for continue signal...
[task 2022-01-21T00:47:12.950Z] 00:47:12 INFO - <<<<<<<
[task 2022-01-21T00:47:15.205Z] 00:47:15 WARNING - PROCESS-CRASH | devtools/shared/heapsnapshot/tests/xpcshell/test_HeapAnalyses_takeCensus_04.js | application crashed [@ mozilla::dom::ChromeUtils::SaveHeapSnapshotShared(mozilla::dom::GlobalObject&, mozilla::dom::HeapSnapshotBoundaries const&, nsTSubstring<char16_t>&, nsTSubstring<char16_t>&, mozilla::ErrorResult&)]

Backout: https://hg.mozilla.org/integration/autoland/rev/2b284e2021238133e06360fbd9780cf7c54d0f9f

Pushed by sfink@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/8c0cf85100bd Generalize traversing through a function's CFG with BFS_upwards(). r=jonco https://hg.mozilla.org/integration/autoland/rev/7e9997f43382 Move some CFG-traversing code from analyzeRoots.js to the common CFG.js, to prepare for calling it from other phases of the analysis. r=jonco https://hg.mozilla.org/integration/autoland/rev/696487e1deea Switch to command-line argument parser. r=jonco https://hg.mozilla.org/integration/autoland/rev/578eafe2d4ef Generate gcEdges file with computeCallgraph instead of computeGCFunctions r=jonco https://hg.mozilla.org/integration/autoland/rev/16083f3b9c4e Annotate edges that invoke destructors on ref-counted values that can be proven to not Release r=jonco https://hg.mozilla.org/integration/autoland/rev/893538c007bf Fix the false positive from a loop-scoped GC pointer named temporary r=jonco https://hg.mozilla.org/integration/autoland/rev/8cdc7b80f4cd Switch RootList::init from taking an AutoCheckCannotGC token to returning one r=jonco https://hg.mozilla.org/integration/autoland/rev/108b34a5b7bc Replace the overcomplicated bodyEatsVariable with a quick linear-only test, and improve the comments around std::move + UniquePtr&& handling r=jonco https://hg.mozilla.org/integration/autoland/rev/6fafd8c4cf28 Fix return value handling, specifically when the final edge in a function body GCs and is treating as using the return value. (This fixes false alarms when returning nullptr in the scope of a GC'ing RAII destructor.) r=jonco https://hg.mozilla.org/integration/autoland/rev/22f66843f6fa arg0_holder can have a non *Argument type, eg if it's using SpiderMonkeyInterfaceRooter. r=jonco
Flags: needinfo?(sphink)
Blocks: 1690111
See Also: → 1579303
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: