1600647 - Clarify where strong references should be acquired

Status: NEW

(Core :: Storage: IndexedDB, task, P2)

Description

[Simon Giesecke [:sg] [he/him]]

Based on the discussion in https://phabricator.services.mozilla.com/D55276#inline-335333, it seems necessary to clarify when strong references must be acquired.

Some points to consider:

• It is probably necessary to balance safety and efficiency. The most safe approach is probably not sufficiently efficient.
• Where safety is valued over efficiency, it must be ensured that this is done consistently, such that safety is not compromised.
• Is there a difference between RefPtr and nsCOMPtr in this regard?

Comment 1

[Andrew Sutherland [:asuth] (he/him)]

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

• Is there a difference between RefPtr and nsCOMPtr in this regard?

https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/nsCOMPtr_versus_RefPtr

Comment 2

[Simon Giesecke [:sg] [he/him]]

(In reply to Andrew Sutherland [:asuth] (he/him) from comment #1)

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

• Is there a difference between RefPtr and nsCOMPtr in this regard?

https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/nsCOMPtr_versus_RefPtr

FWIU, it doesn't mention any differences w.r.t. to handling of refcounting, although it does not explicitly say so. I added this reference to https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/nsCOMPtr_versus_RefPtr though, as it exactly explains what's briefly mentioned there.

Comment 3

[Simon Giesecke [:sg] [he/him]]

Some specific cases:

struct X {
  RefPtr<Foo> mFoo;
};

class Y {
  RefPtr<Baz> mBaz;
public:
  Baz &Baz() const {
    MOZ_ASSERT(mBaz);
    return *mBaz;
  }

  Baz *GetBazNonOwningPtr() const {
    return mBaz;
  }

  RefPtr<Baz> GetBazOwningPtr() const {
    return mBaz;
  }
};

class Z {
  RefPtr<Bar> mBar;
  RefPtr<X> mX; 
  RefPtr<Y> mY;
public:
  void DoSomethingWithBar() {
     RefPtr<Bar> bar = mBar; bar->DoSomething(); // A1
     
     mBar->DoSomething(); // A2
  }

  void DoSomethingWithFoo() {
     RefPtr<Foo> foo = mX->mFoo; foo->DoSomething(); // B1
     
     mX->mFoo->DoSomething(); // B2
  }

  void DoSomethingWithBaz() {
     RefPtr<Baz> baz = mY->GetBazOwningPtr(); baz->DoSomething(); // C1

     mY->Baz().DoSomething(); // C2a

     mY->GetBazNonOwningPtr()->DoSomething(); // C2b

     mY->GetBazOwningPtr()->DoSomething(); // C2c

     RefPtr<Baz> baz = mY->GetBazNonOwningPtr(); baz->DoSomething(); // C3
  }
  
};

Which of these variants should be allowed/preferred?

C3 is dangerous (initializing a RefPtr from a non-owning ptr) and should be disallowed (however I have seen this frequently), and we should have static analysis to find such uses.

C2c is equivalent to C1, but it hides the fact that the RefPtr is copied (it is visible here only because of the specific name of the function), and we should have static analysis to find such uses.

I would prefer C2a over C2b somehow.

This leaves the question if A1/B1/C1 or A2/B2/C2a should be used. I think a consistent rule must answer this likewise for A/B/C, since regarding use-after-free safety, there is no difference between A/B/C. Always doing A1 (and therefore also B1/C1) seems to be too inefficient (and hard to read). Therefore I think, usually A2/B2/C2a should be done, except if there is sufficient indication that the call might involve some reentrancy, possible through calling into arbitrary JS code, which might lead to all non-local references to the object that is called upon being released. This condition should be clearly marked via an explaining comment (and probably it should be attempted to be redesigned to avoid this altogether).

Side question: Would it make any difference if mX and mY were no RefPtr<X>/RefPtr<Y>s but X and Y values? (I don't think so).