1186007 - Prevent calling DisconnectFromOwner from anywhere but nsGlobalWindow

Status: RESOLVED INVALID

(Core :: DOM: Core & HTML, defect)

Description

[(no longer active)]

Right now this function can be called from anywhere, and doing so will result in things similar to bug 1176898.

There is a neat trick that we can employ to make this impossible to happen, see the program below for example:

class Base {
  friend class Friend;
  struct Secret {};
public:
  struct Token {
    Token(const Secret&) {}
  };
  virtual void DisconnectFromOwner(const Token& token) {};
};

struct Derived : Base {
  virtual void DisconnectFromOwner(const Token& token) {
    foo();
    Base::DisconnectFromOwner(token);
  }
  void foo() {};
};

struct Friend {
  void test() {
    Derived d;
    d.DisconnectFromOwner(Base::Token(Base::Secret()));
  }
};

struct NonFriend {
  void test() {
    Derived d;
    d.DisconnectFromOwner(Base::Token(Base::Secret()));
  }
};

Compiling this will give:

$ clang -c test.cpp
test.cpp:29:45: error: 'Secret' is a private member of 'Base'
    d.DisconnectFromOwner(Base::Token(Base::Secret()));
                                            ^
test.cpp:3:10: note: implicitly declared private here
  struct Secret {};
         ^
1 error generated.

Which is what we want.

The trick is to make DisconnectFromOwner take a const Token& argument, to make it require a Token object constructed on the stack somehow before you can call that function.  The Token type itself should be world visible so that derived versions of DisconnectFromOwner can be declared.  But the constructor of the Token type will take a reference to a Secret type which is a private member of DOMEventTargetHelper, and DETH will friend nsGlobalWindow.  This will effectively make DisconnectFromOwner uncallable from elsewhere.

Comment 1

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

IIRC this is how bent's FriendKey stuff works.

Comment 2

[(no longer active)]

Yes, looks like that's exactly how it works!

Comment 3

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

:smaug, I think we categorically want to support removing GlobalTeardownObservers before the global they are associated with is terminated (or they are GCed which seems to be a flaw in this bug's rationale), do you agree? In which case we can mark this bug WONTFIX or maybe INVALID. My particular motivation for asking is that PerformanceObserver seems like it could be cleaned up to be a GlobalTeardownObserver and where the semantics around Disconnect removing the observer from a list of observers would most line up with calling DisconnectFromOwner at an arbitrary time[1].

I believe the motivation of this bug to be that at the time of bug 1176898 DisconnectEventTargetObjects did not handle if a registered DETH was removed during disconnection of the DETHs. There is an analysis in https://bugzilla.mozilla.org/show_bug.cgi?id=1176898#c20 that seems to allege the problem was a DETH subclass no longer had a valid mOwner causing problems, but I don't see how that could happen, whereas since the crash was during disconnection, it makes sense that one disconnecting object disconnecting another DETH during its disconnection could explain what was observed.

Because of the logic in nsIGlobalObject::ForEachGlobalTeardownObserver it's no longer a problem if DETHs are removed during disconnection. (It's also fairly likely most things would be cycle collected and the snow white deletion deferral would also moot the problem, but that's a separate thing.)

void nsIGlobalObject::ForEachGlobalTeardownObserver(
    const std::function<void(GlobalTeardownObserver*, bool* aDoneOut)>& aFunc)
    const {
  // Protect against the function call triggering a mutation of the list
  // while we are iterating by copying the DETH references to a temporary
  // list.
  AutoTArray<RefPtr<GlobalTeardownObserver>, 64> targetList;
  for (const GlobalTeardownObserver* gto = mGlobalTeardownObservers.getFirst();
       gto; gto = gto->getNext()) {
    targetList.AppendElement(const_cast<GlobalTeardownObserver*>(gto));
  }

1: Specifically, the spec language there around "list of things associated with a global" lines up with the nsIGlobalObject::ForEachGlobalTeardownObserver helper that gets used by ServiceWorkers to look up existing ServiceWorker and ServiceWorkerRegistration instances on the global that I imagine could come up other places as well where it would be nice to directly remove the object from being associated with the global at the time it becomes inert rather than waiting for it to (hopefull) be GC'ed.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Yeah, we don't want this bug.