Bug 210555 - REGRESSION (r258977): Crash under Document::visibilityStateChanged
Summary: REGRESSION (r258977): Crash under Document::visibilityStateChanged
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on:
Blocks: 208516
  Show dependency treegraph
 
Reported: 2020-04-15 10:24 PDT by Chris Dumez
Modified: 2020-04-15 11:54 PDT (History)
5 users (show)

See Also:


Attachments
Patch (1.50 KB, patch)
2020-04-15 10:25 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2020-04-15 10:24:24 PDT
Crash under Document::visibilityStateChanged:
[  0] 0x0000000107974669 WebCore`WebCore::Document::visibilityStateChanged() + 409 at Document.cpp:1749
       1745	        client->visibilityStateChanged();
       1746	
       1747	#if ENABLE(MEDIA_STREAM)
       1748	    if (hidden()) {
    -> 1749	        RealtimeMediaSourceCenter::singleton().setCapturePageState(hidden(), page()->isMediaCaptureMuted());
       1750	        return;
       1751	    }
       1752	#if PLATFORM(IOS_FAMILY)
       1753	    if (!PlatformMediaSessionManager::sharedManager().isInterrupted())
Comment 1 Chris Dumez 2020-04-15 10:25:51 PDT
Created attachment 396546 [details]
Patch
Comment 2 Chris Dumez 2020-04-15 10:26:59 PDT
I am not quite sure how to write a test for this yet. If anybody has an idea, please let me know.
Comment 3 Chris Dumez 2020-04-15 10:28:33 PDT
(In reply to Chris Dumez from comment #2)
> I am not quite sure how to write a test for this yet. If anybody has an
> idea, please let me know.

Looks like there is an internals.setPageVisibility() method. I will try that we a detached iframe document to see if it reproduces.
Comment 4 Chris Dumez 2020-04-15 10:51:56 PDT
Some JS event must get fired synchronously when Document::visibilityStateChanged() is called. The "visibilitychange" is fired asynchronously so I wasn't able to write a test based on this event. I believe a JS event (likely media related) gets fired synchronously and the event handler in JS removes one of the iframes from the document.
Comment 5 youenn fablet 2020-04-15 10:56:14 PDT
Could it be the muted event of a MediaStreamTrack?
On iOS, if document goes in the background, a video capture track gets muted.
Comment 6 youenn fablet 2020-04-15 10:58:07 PDT
(In reply to youenn fablet from comment #5)
> Could it be the muted event of a MediaStreamTrack?
> On iOS, if document goes in the background, a video capture track gets muted.

Nope, since this would happen after the setCapturePageState call.
Comment 7 Chris Dumez 2020-04-15 11:05:10 PDT
(In reply to youenn fablet from comment #6)
> (In reply to youenn fablet from comment #5)
> > Could it be the muted event of a MediaStreamTrack?
> > On iOS, if document goes in the background, a video capture track gets muted.
> 
> Nope, since this would happen after the setCapturePageState call.

I don't think the order matters. Page::forEachDocument() gathers all the documents of the page in a Vector. Then iterates over this vector and calls Document::visibilityStateChanged() on each document. The first document is the top document.

So if the top document has an event listener for any event that gets called synchronously during Document::visibilityStateChanged() and if that event listener removes a subframe from the document then we would hit this crash when calling Document::visibilityStateChanged() for the subframe document.
Comment 8 Chris Dumez 2020-04-15 11:38:20 PDT
(In reply to Chris Dumez from comment #7)
> (In reply to youenn fablet from comment #6)
> > (In reply to youenn fablet from comment #5)
> > > Could it be the muted event of a MediaStreamTrack?
> > > On iOS, if document goes in the background, a video capture track gets muted.
> > 
> > Nope, since this would happen after the setCapturePageState call.
> 
> I don't think the order matters. Page::forEachDocument() gathers all the
> documents of the page in a Vector. Then iterates over this vector and calls
> Document::visibilityStateChanged() on each document. The first document is
> the top document.
> 
> So if the top document has an event listener for any event that gets called
> synchronously during Document::visibilityStateChanged() and if that event
> listener removes a subframe from the document then we would hit this crash
> when calling Document::visibilityStateChanged() for the subframe document.

HTMLMediaElement::visibilityStateChanged() gets called. Any idea if this can fire an event synchronously?
Comment 9 EWS 2020-04-15 11:53:07 PDT
Committed r260142: <https://trac.webkit.org/changeset/260142>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 396546 [details].
Comment 10 Radar WebKit Bug Importer 2020-04-15 11:54:19 PDT
<rdar://problem/61840165>