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

• Status: RESOLVED
• Resolution: FIXED
• Component: Bindings

[Chris Dumez]

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.

[Chris Dumez]

Created attachment 395219 [details]
Patch

[Chris Dumez]

Created attachment 395223 [details]
Patch

[Alexey Proskuryakov]

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

[Chris Dumez]

Created attachment 395268 [details]
Patch

[Chris Dumez]

(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.

[Chris Dumez]

Created attachment 395280 [details]
Patch

[Chris Dumez]

Created attachment 395283 [details]
Patch

[Ryosuke Niwa]

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?

[Chris Dumez]

(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.

[Chris Dumez]

Created attachment 395299 [details]
Patch

[Chris Dumez]

Comment on attachment 395299 [details]
Patch

Clearing flags on attachment: 395299

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

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.

[Ryosuke Niwa]

(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.

[Radar WebKit Bug Importer]

<rdar://problem/61227728>