Bug 196774 – Regression(r240562) Audio sometimes keeps playing in previous process after a process-swap

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Chris Dumez]

Audio sometimes keeps playing in previous process after a process-swap since r240562.

[Chris Dumez]

<rdar://problem/49460572>

[Chris Dumez]

Created attachment 367136 [details]
Patch

[Chris Dumez]

Created attachment 367141 [details]
Patch

[Alex Christensen]

Comment on attachment 367141 [details]
Patch

This looks good.  I lament the lack of tests.

[Chris Dumez]

(In reply to Alex Christensen from comment #4)
> Comment on attachment 367141 [details]
> Patch
> 
> This looks good.  I lament the lack of tests.

Yes, I wasn't able to find a way to API test this.

[Chris Dumez]

Comment on attachment 367141 [details]
Patch

Clearing flags on attachment: 367141

Committed r244144: <https://trac.webkit.org/changeset/244144>

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.

[Ryan Haddad]

(In reply to Chris Dumez from comment #5)
> (In reply to Alex Christensen from comment #4)
> > Comment on attachment 367141 [details]
> > Patch
> > 
> > This looks good.  I lament the lack of tests.
> 
> Yes, I wasn't able to find a way to API test this.
It did manage to break one though, as shown by EWS:
https://ews-build.webkit.org/#/builders/3/builds/385

[Chris Dumez]

(In reply to Ryan Haddad from comment #8)
> (In reply to Chris Dumez from comment #5)
> > (In reply to Alex Christensen from comment #4)
> > > Comment on attachment 367141 [details]
> > > Patch
> > > 
> > > This looks good.  I lament the lack of tests.
> > 
> > Yes, I wasn't able to find a way to API test this.
> It did manage to break one though, as shown by EWS:
> https://ews-build.webkit.org/#/builders/3/builds/385

Will look now.

[Chris Dumez]

(In reply to Chris Dumez from comment #9)
> (In reply to Ryan Haddad from comment #8)
> > (In reply to Chris Dumez from comment #5)
> > > (In reply to Alex Christensen from comment #4)
> > > > Comment on attachment 367141 [details]
> > > > Patch
> > > > 
> > > > This looks good.  I lament the lack of tests.
> > > 
> > > Yes, I wasn't able to find a way to API test this.
> > It did manage to break one though, as shown by EWS:
> > https://ews-build.webkit.org/#/builders/3/builds/385
> 
> Will look now.

Looks like that API test is flaky.

[Chris Dumez]

Comment on attachment 367141 [details]
Patch

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

> Source/WebKit/UIProcess/SuspendedPageProxy.cpp:172
> +        m_process->processPool().removeSuspendedPage(*this); // Will destroy |this|.

I introduced an unintentional behavior change here by destroying the SuspendedPageProxy instead of closing it. Fixing via https://bugs.webkit.org/show_bug.cgi?id=196784

> Source/WebKit/UIProcess/WebPageProxy.cpp:-7091
> -    m_process->processPool().closeFailedSuspendedPagesForPage(*this);

Note that this was closing the SuspendedPage, ...

> Source/WebKit/UIProcess/WebProcessPool.cpp:-2409
> -            suspendedPage->close();

... here, not destroying it.

[Chris Dumez]

Follow up fix landed in https://trac.webkit.org/changeset/244186