1478101 - Stop using the no-arg DOMEventTargetHelper constructor from AbortSignal

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement, P2)

Description

[Boris Zbarsky [:bzbarsky]]

Searchfox doesn't  know about it, because it's an implicit call (see bug 1478097).  But it's there.

Andrea, which globals should these AbortSignals be associated with?  The relevant callsites are:

  AbortSignalProxy::GetOrCreateSignalForMainThread
  Request::Request
  Request::GetOrCreateSignal

Comment 1

[Andrea Marchesini [:baku]]

>   AbortSignalProxy::GetOrCreateSignalForMainThread

This one is not associated with any particular global: that method creates a AbortSignal object on the main-thread to follow the AbortSignal::abort() calls executed on the worker thread.

>   Request::Request
>   Request::GetOrCreateSignal

This should probably use the global of the Request object.

Comment 2

[Andrea Marchesini [:baku]]

Attachment: abort.patch

Comment 3

[Boris Zbarsky [:bzbarsky]]

> This one is not associated with any particular global

What happens when events are dispatched on it?  Do events get dispatched on it?

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8994746 [details] [diff] [review]
abort.patch

I believe the goal in bug 1452705 is to not allow passing an null global to DETH.

Comment 5

[Andrea Marchesini [:baku]]

Attachment: part 2 - no dispatching events

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8994746 [details] [diff] [review]
abort.patch

I guess my real question was whether this not-associated-with-a-global thing should inherit from DETH at all.  And if so how we want to address the problems bug 1452705 talks about.

Initing with null does not solve the problem this bug is filed on; we might as well keep using the no-arg constructor...

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8994750 [details] [diff] [review]
part 2 - no dispatching events

Seems OK, though again the real question is whether AbortSignal should just be split up into two classes or something.

Comment 8

[Andrea Marchesini [:baku]]

Attachment: part 1 - no mController in AbortSignal

Comment 9

[Andrea Marchesini [:baku]]

Attachment: part 2 - AbortSignalImpl

Comment 10

[Andrea Marchesini [:baku]]

Attachment: abort_1.patch

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8996273 [details] [diff] [review]
abort_1.patch

Thank you for going through all this!

>+  for (uint32_t i = 0; i < mFollowers.Length(); ++i) {

I know you just copied this, but why not a ranged for?

> NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN_INHERITED(AbortSignal,

So.. shouldn't we traverse/unlink mFollowingSignal?  I know we don't right now; maybe a followup to fix that?

Similar for other subclasses of AbortFollower

>+  virtual void
>   Abort() override;

No "virtual", since it's already "override".

>-  NS_IMPL_CYCLE_COLLECTION_UNLINK(mSignal)

Why does Response not need to traverse/unlink mSignalImpl?

>+++ b/dom/webauthn/WebAuthnManager.cpp

Why are the changes here needed at all?   The places that were getting an AbortSignal from the caller can keep working with it; passing an AbortSignal* to Follow(), the WebAuthnTransaction ctor, etc should work fine...

That is, in the places where we know we have an AbortSignal, we should just have the type reflect that.

Comment 12

[Andrea Marchesini [:baku]]

Attachment: abort_2.patch

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8996960 [details] [diff] [review]
abort_2.patch

>+++ b/dom/fetch/Response.cpp
>+  tmp->mSignalImpl = nullptr;

Why is that not using the CC macro.  And why are we not traversing mSignalImpl?

>+++ b/dom/fetch/Response.cpp
>+  // is used. Otherwise, mSignalImpl is used. These 2 members are mutual
>+  // exclusive.

"mutually exclusive"

Why do we need them to be mutually exclusive?  Couldn't we always assign mSignalImpl and assign mSignal as needed?

But I'm still not clear on why we need mSignal at all.  Why do we?

Comment 14

[Andrea Marchesini [:baku]]

Attachment: abort_2.patch

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8998063 [details] [diff] [review]
abort_2.patch

>+++ b/dom/abort/AbortSignal.cpp
>+  NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, DOMEventTargetHelper)

Why do you need this?  Just chaining up the QI to DOMEventTargetHelper as we were already doing should have the same effect...

>+++ b/dom/abort/AbortSignal.h
>+  // Any implementation of this AbortFollower must Traverse/Unlink this member.

 // Subclasses of AbortFollower must Traverse/Unlink this member.

>+// Any implementation of this class must Traverse/Unlink mFollowingSignal.

  // Any subclass of this class must Traverse/Unlink mFollowingSignal.

AbortSignalProxy probably needs to traverse/unlink mFollowingSignal given those comments.  Followup bug for that, please?  Same for FetchObserver, etc.  Or is that tracked in bug 1480366?

r=me.  Sorry for the lag again.  :(

Comment 16

[Andrea Marchesini [:baku]]

> AbortSignalProxy probably needs to traverse/unlink mFollowingSignal given
> those comments.  Followup bug for that, please?  Same for FetchObserver,
> etc.  Or is that tracked in bug 1480366?

Yes, I'm planning to traverse/unlink mFollowingSignal in bug 1480366, but not everywhere: AbortSignalProxy is just a temporary object used during the fetch operation. This is fine to be out of CC.

Comment 17

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/2d0ebe0ace0b