Bug 209886

Summary:

ActiveDOMObject::hasPendingActivity() should stop preventing wrapper collection after ActiveDOMObject::stop() has been called

Product:

WebKit

Reporter:

Chris Dumez <cdumez>

Component:

Bindings

Assignee:

Chris Dumez <cdumez>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

alecflett, ap, beidson, cdumez, eric.carlson, esprehn+autocc, ews-watchlist, ggaren, glenn, gyuyoung.kim, hta, jer.noble, jsbell, kangil.han, macpherson, menard, philipj, rniwa, sergio, tommyw, webkit-bug-importer, ysuzuki

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

See Also:

https://bugs.webkit.org/show_bug.cgi?id=209754

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none

Description Chris Dumez 2020-04-01 16:13:56 PDT

ActiveDOMObject::hasPendingActivity() should never return true after stop() has been called. ActiveDOMObject::stop() gets called when the script execution context is about to get destroyed. At this point, we should no longer be running script so there is never any need to keep the JS wrapper alive. As a result, ActiveDOMObject::hasPendingActivity() should be returning false after stop() has been called. This will make it harder to cause JS wrapper leaks with ActiveDOMObjects.

Comment 1 Chris Dumez 2020-04-01 16:15:36 PDT

Created attachment 395219 [details]
Patch

Comment 2 Chris Dumez 2020-04-01 16:27:08 PDT

Created attachment 395223 [details]
Patch

Comment 3 Alexey Proskuryakov 2020-04-01 17:40:08 PDT

Is it possible for a script to run in another frame, and to reference something in the stopped one?

Comment 4 Chris Dumez 2020-04-02 08:43:32 PDT

Created attachment 395268 [details]
Patch

Comment 5 Chris Dumez 2020-04-02 09:05:03 PDT

(In reply to Alexey Proskuryakov from comment #3)
> Is it possible for a script to run in another frame, and to reference
> something in the stopped one?

AFAIK, an ActiveDOMObject should really not run script after stop() has been called. I think that if events were still fired after stop() has been called, it would be a bug.

Comment 6 Chris Dumez 2020-04-02 10:59:27 PDT

Created attachment 395280 [details]
Patch

Comment 7 Chris Dumez 2020-04-02 11:49:00 PDT

Created attachment 395283 [details]
Patch

Comment 8 Ryosuke Niwa 2020-04-02 14:19:49 PDT

Comment on attachment 395283 [details]
Patch

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

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:4777
> -            push(@implContent, "    if (js${interfaceName}->wrapped().hasPendingActivity()) {\n");
> +            push(@implContent, "    if (!js${interfaceName}->wrapped().isContextStopped() && js${interfaceName}->wrapped().hasPendingActivity()) {\n");

Can we add a helper function like hasPendingActivityAndNotStopped so that we don't do de-referencing twice here?

Comment 9 Chris Dumez 2020-04-02 14:20:53 PDT

(In reply to Ryosuke Niwa from comment #8)
> Comment on attachment 395283 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=395283&action=review
> 
> > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:4777
> > -            push(@implContent, "    if (js${interfaceName}->wrapped().hasPendingActivity()) {\n");
> > +            push(@implContent, "    if (!js${interfaceName}->wrapped().isContextStopped() && js${interfaceName}->wrapped().hasPendingActivity()) {\n");
> 
> Can we add a helper function like hasPendingActivityAndNotStopped so that we
> don't do de-referencing twice here?

I can also add a stack variable in before the if check for js${interfaceName}->wrapped(). I figured it did not matter much because it is generated code.

Comment 10 Chris Dumez 2020-04-02 14:26:29 PDT

Created attachment 395299 [details]
Patch

Comment 11 Chris Dumez 2020-04-02 14:49:49 PDT

Comment on attachment 395299 [details]
Patch

Clearing flags on attachment: 395299

Committed r259419: <https://trac.webkit.org/changeset/259419>

Comment 12 Chris Dumez 2020-04-02 14:49:51 PDT

All reviewed patches have been landed.  Closing bug.

Comment 13 Ryosuke Niwa 2020-04-02 14:49:55 PDT

(In reply to Chris Dumez from comment #9)
> (In reply to Ryosuke Niwa from comment #8)
> > Comment on attachment 395283 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=395283&action=review
> > 
> > > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:4777
> > > -            push(@implContent, "    if (js${interfaceName}->wrapped().hasPendingActivity()) {\n");
> > > +            push(@implContent, "    if (!js${interfaceName}->wrapped().isContextStopped() && js${interfaceName}->wrapped().hasPendingActivity()) {\n");
> > 
> > Can we add a helper function like hasPendingActivityAndNotStopped so that we
> > don't do de-referencing twice here?
> 
> I can also add a stack variable in before the if check for
> js${interfaceName}->wrapped(). I figured it did not matter much because it
> is generated code.

Generated code or not, it would incur runtime cost.

Comment 14 Radar WebKit Bug Importer 2020-04-02 14:50:17 PDT

<rdar://problem/61227728>