Restrict MOZ_KNOWN_LIVE to be modifiable on constructers/destructors
Categories
(Developer Infrastructure :: Source Code Analysis, task)
Tracking
(firefox102 fixed)
Tracking | Status | |
---|---|---|
firefox102 | --- | fixed |
People
(Reporter: saschanaz, Assigned: saschanaz)
References
(Depends on 1 open bug, Blocks 1 open bug)
Details
Attachments
(3 files)
:smaug said that using MOZ_KNOWN_LIVE is not appropriate in a non-stack class as an incomplete unlink can break the assumption. Fortunately some uses are on stack classes but I think not all of them are. We need to adjust the linter.
(Probably MOZ_KnownLive check should also be adjusted, but perhaps it's easier to migrate to MOZ_KNOWN_LIVE.)
Assignee | ||
Comment 1•3 years ago
•
|
||
I think I didn't fully understand the situation.
class Foo {
MOZ_KNOWN_LIVE RefPtr<Bar> mBar;
};
MOZ_CAN_RUN_SCRIPT funcBar(Bar* aBar);
MOZ_CAN_RUN_SCRIPT funcFoo(Foo* aFoo) {
funcBar(aFoo->mBar);
}
Here, passing mBar
without doing RefPtr<Bar> bar = mBar;
should be safe because the caller should grab the aFoo
and mBar
and thus unlinking shouldn't happen at all, right?
Maybe this is only problematic when accessing mBar
inside non-MOZ_CAN_RUN_SCRIPT context because nothing makes sure Foo
is grabbed in that case? (But then is it really problematic? MOZ_KNOWN_LIVE is only ever meaningful within MOZ_CAN_RUN_SCRIPT.)
Assignee | ||
Comment 2•3 years ago
•
|
||
Maybe just rename it as MOZ_LIVE_BEFORE_CC
or such? (MOZ_IMMUTABLE_EXCEPT_CC
? 🤔)
Comment 3•3 years ago
•
|
||
I didn't say it is not appropriate for non-stack classes. If the class isn't cycle collectable at all, for example many Runnable classes, then use of MOZ_KNOWN_LIVE should be totally fine. The variable is set in the constructor and then after destructor it will be cleared.
Other option is some non-CCed member variable in a CCable class, that might stay alive always - depends on the case.
Updated•3 years ago
|
Assignee | ||
Comment 4•3 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #3)
I didn't say it is not appropriate for non-stack classes. If the class isn't cycle collectable at all, for example many Runnable classes, then use of MOZ_KNOWN_LIVE should be totally fine. The variable is set in the constructor and then after destructor it will be cleared.
Other option is some non-CCed member variable in a CCable class, that might stay alive always - depends on the case.
That sounds like CC-able members on non-stack classes are still problematic, but in MOZ_CAN_RUN_SCRIPT the class should be grabbed by callers and thus CC should never occur, no?
Assignee | ||
Comment 5•3 years ago
•
|
||
I chatted with :smaug offline.
MOZ_KNOWN_LIVE is confusing since it certainly won't be live after cycle collection. Making it MOZ_IMMUTABLE and MOZ_IMMUTABLE_OUTSIDE_CC is better as:
- MOZ_IMMUTABLE says it's not to be mutated.
- MOZ_IMMUTABLE_OUTSIDE_CC says cycle collection can still mutate it. This works same as MOZ_IMMUTABLE but exists separately because stack classes are nothing to do with CC.
Ideally we could also introduce a checker for MOZ_IMMUTABLE to be actually immutable outside certain initialization methods and Unlink()
.
Comment 6•3 years ago
|
||
I know I'm saying an edge case though, even if a class in the stack, it can be revered by pointer if the point is in somewhere outside the stack. Then, anybody can touch its members. So in my understanding, treating strong pointer in the stack as safe is a result of trade off with avoiding most usual mistakes vs. doing the check too strictly.
So, not limiting to MOZ_STACK_CLASS
is not better thing to me. Instead, it's storage duration should be checked.
On the other hand, the CC issue makes me have a headache. Some non-Get
methods return members which are registered in the CC. So, it sounds reasonable to make them call "immutable" instead of "safe". Then, static analysis could prevent mutation of the member including by the CC. And perhaps, if raw pointer or reference should be marked as safe, they should be used with "immutable" and makes its initializer limited to only MOZ_CAN_RUN_SCRIPT
methods and the source should be limited to its argument.
Assignee | ||
Comment 7•3 years ago
•
|
||
even if a class in the stack, it can be revered by pointer if the point is in somewhere outside the stack. Then, anybody can touch its members.
I had a suggestion for this:
class MyClass {
MOZ_IMMUTABLE RefPtr<Foo> mFoo;
}
static void Foo() {
MyClass myClass;
/* ... */
Bar(&myClass);
Baz(&myClass);
/* ... */
}
static void Bar(MyClass& myClass) {
myClass.mFoo = nullptr; // This field must not change after initialization! Add MOZ_INITIALIZER if you are initializing it.
}
MOZ_INITIALIZER static void Baz(MyClass& myClass) {
myClass.mFoo = nullptr; // Ok
}
This does not prevent calling Bar()
again but could be useful to prevent random things from touching members.
Edit: Perhaps it would be better to constrain it to real C++ constructors (and Unlink()
for CC`) though.
Assignee | ||
Updated•3 years ago
|
Assignee | ||
Comment 8•3 years ago
|
||
Updated•3 years ago
|
Assignee | ||
Comment 9•3 years ago
•
|
||
HTMLEditor::CellData::mElement violates the new proposed rule, it's nullified and set again outside construtor/destructor. Do you think it's okay to block that? (Or perhaps the rule can special-case MOZ_STACK_CLASS members.)
Comment 10•3 years ago
|
||
(In reply to Kagami :saschanaz from comment #9)
HTMLEditor::CellData::mElement violates the new proposed rule, it's nullified and set again outside construtor/destructor. Do you think it's okay to block that? (Or perhaps the rule can special-case MOZ_STACK_CLASS members.)
Ah, it must be safe for current users, but it's not from point of view of static analysis. I think that it shoud be redesigned to RAII class. I'll write a patch for it, wait a moment...
Updated•3 years ago
|
Assignee | ||
Comment 11•3 years ago
|
||
Those are indeed not being mutated during the object's lifetime before
CC, but currently the initialization is being done outside of the
constructor and that violates the newly proposed rule.
The only affected use is TransformStreamDefaultController::mStream
though, as others are exposed by public getters which does not support
MOZ_KNOWN_LIVE yet (and thus MOZ_KnownLive() is used by callers).
Ideally those should be refactored to be initialized within the
constructor, but I'll do that separately.
Depends on D146023
Assignee | ||
Comment 12•3 years ago
|
||
The comment is lying as nothing actually uses it.
Depends on D146209
Comment 13•3 years ago
|
||
Comment 14•3 years ago
|
||
Backed out for causing build bustages on Transferable.cpp
Backout link: https://hg.mozilla.org/integration/autoland/rev/8caa1a11e47842441b30502d9ad235228bc3d918
Link to failure log:
https://treeherder.mozilla.org/logviewer?job_id=378498882&repo=autoland&lineNumber=37608
https://treeherder.mozilla.org/logviewer?job_id=378498921&repo=autoland&lineNumber=19410
Failure line :
/builds/worker/checkouts/gecko/dom/streams/Transferable.cpp:1017:8: error: arguments must all be strong refs or caller's parameters when calling a function marked as MOZ_CAN_RUN_SCRIPT (including the implicit object argument). 'mReadable->' is neither.
make[4]: *** [/builds/worker/checkouts/gecko/config/rules.mk:659: Unified_cpp_dom_streams0.obj] Error 1
Comment 15•3 years ago
|
||
Comment 16•3 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/1f14716a95ae
https://hg.mozilla.org/mozilla-central/rev/b0eba0e09620
https://hg.mozilla.org/mozilla-central/rev/fafa0eba0916
Assignee | ||
Updated•3 years ago
|
Assignee | ||
Comment 17•3 years ago
|
||
(No rename happened. I'm not sure immutable is right here since the subtype of RefPtr is still mutable. Maybe just const, since we know const pointer allows mutation?)
Updated•2 years ago
|
Description
•