Stop using the no-arg DOMEventTargetHelper constructor from AbortSignal

RESOLVED FIXED in Firefox 63

Status

()

enhancement
P2
normal
RESOLVED FIXED
10 months ago
2 months ago

People

(Reporter: bzbarsky, Assigned: baku)

Tracking

(Blocks 1 bug)

unspecified
mozilla63
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox63 fixed)

Details

Attachments

(1 attachment, 6 obsolete attachments)

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
Flags: needinfo?(amarchesini)
(Assignee)

Comment 1

10 months ago
>   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.
Flags: needinfo?(amarchesini)
(Assignee)

Comment 2

10 months ago
Posted patch abort.patch (obsolete) — Splinter Review
Assignee: nobody → amarchesini
Attachment #8994746 - Flags: review?(bzbarsky)
(Reporter)

Comment 3

10 months ago
> This one is not associated with any particular global

What happens when events are dispatched on it?  Do events get dispatched on it?
Flags: needinfo?(amarchesini)
(Reporter)

Comment 4

10 months ago
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.
(Assignee)

Comment 5

10 months ago
Flags: needinfo?(amarchesini)
Attachment #8994750 - Flags: review?(bzbarsky)
(Reporter)

Comment 6

10 months ago
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...
Attachment #8994746 - Flags: review?(bzbarsky)
(Reporter)

Comment 7

10 months ago
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.
Attachment #8994750 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 8

10 months ago
Attachment #8994746 - Attachment is obsolete: true
Attachment #8994750 - Attachment is obsolete: true
Attachment #8995905 - Flags: review?(bzbarsky)
(Assignee)

Comment 9

10 months ago
Posted patch part 2 - AbortSignalImpl (obsolete) — Splinter Review
Attachment #8995909 - Flags: review?(bzbarsky)
(Assignee)

Comment 10

10 months ago
Posted patch abort_1.patch (obsolete) — Splinter Review
Attachment #8995905 - Attachment is obsolete: true
Attachment #8995909 - Attachment is obsolete: true
Attachment #8995905 - Flags: review?(bzbarsky)
Attachment #8995909 - Flags: review?(bzbarsky)
Attachment #8996273 - Flags: review?(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.
Attachment #8996273 - Flags: review?(bzbarsky) → review-
(Assignee)

Updated

10 months ago
Depends on: 1480364
(Assignee)

Comment 12

10 months ago
Posted patch abort_2.patch (obsolete) — Splinter Review
Attachment #8996273 - Attachment is obsolete: true
Attachment #8996960 - Flags: review?(bzbarsky)
(Assignee)

Updated

10 months ago
Blocks: 1480366
Priority: -- → P2
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?
Flags: needinfo?(amarchesini)
(Assignee)

Comment 14

10 months ago
Posted patch abort_2.patchSplinter Review
Attachment #8996960 - Attachment is obsolete: true
Attachment #8996960 - Flags: review?(bzbarsky)
Flags: needinfo?(amarchesini)
Attachment #8998063 - Flags: review?(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.  :(
Attachment #8998063 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 16

9 months ago
> 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

9 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/2d0ebe0ace0b
Status: NEW → RESOLVED
Last Resolved: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Depends on: 1486429
Depends on: 1488427
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.