Drop legacy ActiveDOMObject::setPendingActivity() / unsetPendingActivity()

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Chris Dumez]

Drop legacy ActiveDOMObject::setPendingActivity() / unsetPendingActivity(). They are too leak-prone. The modern way to do this is to either override ActiveDOMObject::virtualHasPendingActivity() or use ActiveDOMObject::makePendingActivity().

[Chris Dumez]

Created attachment 430375 [details]
Patch

[Darin Adler]

Comment on attachment 430375 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=430375&action=review

> Source/WebCore/Modules/applepay/ApplePaySession.cpp:791
> +    auto protectedJSWrapper = makePendingActivity(*this);

This object is not itself a JS wrapper. It is an object that protects the wrapper. So naming it "protected JS wrapper" is not quite right.

> Source/WebCore/Modules/mediasource/MediaSource.h:182
> +    unsigned m_associatedRegistryCount { 0 };

Do we have a no-overflow guarantee? 32-bits is a lot of bits, but is it guaranteed enough?

> Source/WebCore/page/EventSource.cpp:278
> +    auto protectedJSWrapper = makePendingActivity(*this);

Same variable name thought.

[Chris Dumez]

(In reply to Darin Adler from comment #2)
> Comment on attachment 430375 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=430375&action=review
> 
> > Source/WebCore/Modules/applepay/ApplePaySession.cpp:791
> > +    auto protectedJSWrapper = makePendingActivity(*this);
> 
> This object is not itself a JS wrapper. It is an object that protects the
> wrapper. So naming it "protected JS wrapper" is not quite right.

jsWrapperProtector?

> 
> > Source/WebCore/Modules/mediasource/MediaSource.h:182
> > +    unsigned m_associatedRegistryCount { 0 };
> 
> Do we have a no-overflow guarantee? 32-bits is a lot of bits, but is it
> guaranteed enough?

I would just like to point out that ActiveDOMObject uses:
 unsigned m_pendingActivityInstanceCount 

So I did not change the situation with this patch. I need to look deeper into the code so see if there is a no-overflow guarantee as I am not familiar with it.

> 
> > Source/WebCore/page/EventSource.cpp:278
> > +    auto protectedJSWrapper = makePendingActivity(*this);
> 
> Same variable name thought.

[Chris Dumez]

(In reply to Chris Dumez from comment #3)
> (In reply to Darin Adler from comment #2)
> > Comment on attachment 430375 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=430375&action=review
> > 
> > > Source/WebCore/Modules/applepay/ApplePaySession.cpp:791
> > > +    auto protectedJSWrapper = makePendingActivity(*this);
> > 
> > This object is not itself a JS wrapper. It is an object that protects the
> > wrapper. So naming it "protected JS wrapper" is not quite right.
> 
> jsWrapperProtector?
> 
> > 
> > > Source/WebCore/Modules/mediasource/MediaSource.h:182
> > > +    unsigned m_associatedRegistryCount { 0 };
> > 
> > Do we have a no-overflow guarantee? 32-bits is a lot of bits, but is it
> > guaranteed enough?
> 
> I would just like to point out that ActiveDOMObject uses:
>  unsigned m_pendingActivityInstanceCount 
> 
> So I did not change the situation with this patch. I need to look deeper
> into the code so see if there is a no-overflow guarantee as I am not
> familiar with it.

Looking at the code, I think JS could potentially cause an overflow by running this in a loop for the same mediaSource object:
`window.URL.createObjectURL(mediaSource)`

Not a regression in my patch. Switching to uint64_t would just delay the inevitable. Not sure how we usually deal with such things.

[Chris Dumez]

Comment on attachment 430375 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=430375&action=review

>>> Source/WebCore/Modules/mediasource/MediaSource.h:182
>>> +    unsigned m_associatedRegistryCount { 0 };
>> 
>> Do we have a no-overflow guarantee? 32-bits is a lot of bits, but is it guaranteed enough?
> 
> I would just like to point out that ActiveDOMObject uses:
>  unsigned m_pendingActivityInstanceCount 
> 
> So I did not change the situation with this patch. I need to look deeper into the code so see if there is a no-overflow guarantee as I am not familiar with it.

This is not a valid use case though so the script would have to have a bug or have bad intentions. The side effect though is only that the wrapper might get collected early (or leak). If a wrapper is collected early, the one consequence I am aware of is the inability to dispatch events.
Do we think we have to deal with overflows here?

[Geoffrey Garen]

> Looking at the code, I think JS could potentially cause an overflow by
> running this in a loop for the same mediaSource object:
> `window.URL.createObjectURL(mediaSource)`
> 
> Not a regression in my patch. Switching to uint64_t would just delay the
> inevitable. Not sure how we usually deal with such things.

I believe we usually deal with this issue by using uint64_t. I think that's how all WK2 object identifiers work. I don't agree that uint64_t delays the inevitable; I believe it is a complete fix, at least for any consumer electronic device in the foreseeable future. [https://www.reddit.com/r/ProgrammerTIL/comments/4tspsn/c_it_will_take_approximately_97_years_to_overflow/]

In some cases, Checked<unsigned> might also be appropriate -- if we are willing to crash when the number gets too big.

[Chris Dumez]

Created attachment 430395 [details]
Patch

[EWS]

Committed r278372 (238399@main): <https://commits.webkit.org/238399@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 430395 [details].

[Radar WebKit Bug Importer]

<rdar://problem/78782275>