Closed Bug 1746090 Opened 1 year ago Closed 1 year 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)
See Also: → 1579303
You need to log in before you can comment on or make changes to this bug.