Closed Bug 1765832 Opened 3 years ago Closed 3 years ago

Restrict MOZ_KNOWN_LIVE to be modifiable on constructers/destructors

Categories

(Developer Infrastructure :: Source Code Analysis, task)

Tracking

(firefox102 fixed)

RESOLVED FIXED
102 Branch
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.)

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.)

Flags: needinfo?(bugs)

Maybe just rename it as MOZ_LIVE_BEFORE_CC or such? (MOZ_IMMUTABLE_EXCEPT_CC? 🤔)

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.

Flags: needinfo?(bugs)

(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?

Flags: needinfo?(bugs)

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:

  1. MOZ_IMMUTABLE says it's not to be mutated.
  2. 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() .

Flags: needinfo?(bugs)
Summary: Make sure MOZ_KNOWN_LIVE is only used in MOZ_STACK_CLASS → Rename MOZ_KNOWN_LIVE to MOZ_IMMUTABLE(_OUTSIDE_CC)

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.

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.

Summary: Rename MOZ_KNOWN_LIVE to MOZ_IMMUTABLE(_OUTSIDE_CC) → Rename MOZ_KNOWN_LIVE to MOZ_IMMUTABLE(_OUTSIDE_CC) and add more restriction
See Also: → 1768251
Assignee: nobody → krosylight
Status: NEW → ASSIGNED

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.)

Flags: needinfo?(masayuki)

(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...

Flags: needinfo?(masayuki)
Attachment #9275979 - Attachment description: WIP: Bug 1765832 - Restrict MOZ_KNOWN_LIVE to be modifiable only by constructor/destructors → Bug 1765832 - Part 1: Restrict MOZ_KNOWN_LIVE to be modifiable only by constructor/destructors r=andi

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

The comment is lying as nothing actually uses it.

Depends on D146209

Pushed by krosylight@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/87cf3ec70aab Part 1: Restrict MOZ_KNOWN_LIVE to be modifiable only by constructor/destructors r=andi https://hg.mozilla.org/integration/autoland/rev/05a53421e1d8 Part 2: Remove redundant MOZ_KNOWN_LIVE in dom/streams r=smaug https://hg.mozilla.org/integration/autoland/rev/c7c5a5208d60 Part 3: Remove AutoSelectionSetterAfterTableEdit::CancelSetCaret r=masayuki

Backed out for causing build bustages on Transferable.cpp

Backout link: https://hg.mozilla.org/integration/autoland/rev/8caa1a11e47842441b30502d9ad235228bc3d918

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception%2Crunnable&revision=c7c5a5208d608b98baaf281aee38721816efaa7f&selectedTaskRun=c6pflr9qTsm1yAiPPDr9lw.0

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

Flags: needinfo?(krosylight)
Pushed by krosylight@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/1f14716a95ae Part 1: Restrict MOZ_KNOWN_LIVE to be modifiable only by constructor/destructors r=andi https://hg.mozilla.org/integration/autoland/rev/b0eba0e09620 Part 2: Remove redundant MOZ_KNOWN_LIVE in dom/streams r=smaug https://hg.mozilla.org/integration/autoland/rev/fafa0eba0916 Part 3: Remove AutoSelectionSetterAfterTableEdit::CancelSetCaret r=masayuki
Flags: needinfo?(krosylight)
Summary: Rename MOZ_KNOWN_LIVE to MOZ_IMMUTABLE(_OUTSIDE_CC) and add more restriction → Rename. MOZ_KNOWN_LIVE to MOZ_IMMUTABLE(_OUTSIDE_CC) and add more restriction

(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?)

Summary: Rename. MOZ_KNOWN_LIVE to MOZ_IMMUTABLE(_OUTSIDE_CC) and add more restriction → Restrict MOZ_KNOWN_LIVE to be modifiable on constructers/destructors
Product: Firefox Build System → Developer Infrastructure
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: