Bug 189021

Created attachment 348223 [details]
Patch

Comment on attachment 348223 [details]
Patch

Attachment 348223 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/9003424

New failing tests:
http/tests/media/collect-media-devices.https.html

Created attachment 348247 [details]
Archive of layout-test-results from ews112 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-sierra  Platform: Mac OS X 10.12.6

Comment on attachment 348223 [details]
Patch

Attachment 348223 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/9004309

New failing tests:
http/tests/media/collect-media-devices.https.html

Created attachment 348251 [details]
Archive of layout-test-results from ews102 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-sierra  Platform: Mac OS X 10.12.6

Created attachment 348255 [details]
Patch

Created attachment 348256 [details]
Patch

Created attachment 348302 [details]
Patch

Created attachment 348328 [details]
Patch for landing

Comment on attachment 348328 [details]
Patch for landing

Clearing flags on attachment: 348328

Committed r235438: <https://trac.webkit.org/changeset/235438>

All reviewed patches have been landed.  Closing bug.

<rdar://problem/43812760>

Comment on attachment 348302 [details]
Patch

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

> Source/WebCore/Modules/mediastream/MediaDevices.cpp:166
>  bool MediaDevices::hasPendingActivity() const
>  {
> -    return scriptExecutionContext() && hasEventListeners(m_eventNames.devicechangeEvent);
> +    return !isContextStopped() && hasEventListeners(m_eventNames.devicechangeEvent);
>  }

Can this new rule be built into ActiveDOMObject instead of of putting it into each override of hasPendingActivity? Or is it not true for all active DOM objects?

> > Source/WebCore/Modules/mediastream/MediaDevices.cpp:166
> >  bool MediaDevices::hasPendingActivity() const
> >  {
> > -    return scriptExecutionContext() && hasEventListeners(m_eventNames.devicechangeEvent);
> > +    return !isContextStopped() && hasEventListeners(m_eventNames.devicechangeEvent);
> >  }
> 
> Can this new rule be built into ActiveDOMObject instead of of putting it
> into each override of hasPendingActivity? Or is it not true for all active
> DOM objects?

I believe we want this to be true and we should build this into ActiveDOMObject.
bug 189018 is a first step in that direction:
- Enforce this to all ActiveDOMObjects that do not override hasPendingActivity
- ASSERT that all ActiveDOMObject::hasPendingActivity return false when context is stopped
If everything succeeds, we could enforce this also for hasPendingActivity overrides.

You could encode this rule mechanically like so:

(*) Make ActiveDOMObject::hasPendingActivity non-virtual, and make it do this:

if (/* document is stopped /*)
    return false;
return hasPendingActivityVirtual();

(*) Rename subclass implementations of hasPendingActivity to hasPendingActivityVirtual.

(*) Remove /* document is stopped */ checks from subclass implementations.

(In reply to Geoffrey Garen from comment #15)
> You could encode this rule mechanically like so:
> 
> (*) Make ActiveDOMObject::hasPendingActivity non-virtual, and make it do
> this:
> 
> if (/* document is stopped /*)
>     return false;
> return hasPendingActivityVirtual();
> 
> (*) Rename subclass implementations of hasPendingActivity to
> hasPendingActivityVirtual.
> 
> (*) Remove /* document is stopped */ checks from subclass implementations.

Sure, one potential issue is that hasPendingActivity be used for other purposes than GC. I was leaning towards adding a non virtual isCollectable method instead.

> Sure, one potential issue is that hasPendingActivity be used for other
> purposes than GC. I was leaning towards adding a non virtual isCollectable
> method instead.

I think that all uses of hasPendingActivity correlate both with GC and document lifetime, and I think I can prove that any use of hasPendingActivity that did not correlate with GC and document lifetime would be a bug.

If you have pending activity but you allow GC, that means that you might fire an event after you collect an object's wrapper and event listeners. That's a compatibility bug.

If you continue pending activity after unloading the document, that means that you have a God-mode activity that the user can't cancel by closing the tab. That's a security / memory leak bug.

Tying document lifetime, GC, and pending activity together in a single function can make that function harder to write -- but it's the good kind of hard because it forces you to ask the hard questions that are required for correct behavior. If isCollectible made it easier not to worry about whether an object had pending activity, it would also make it easier to write incorrect code that collected an object while it still had events to fire, or failed to collect an object even after it was guaranteed not to do any more work or fire any more events.