Bug 196774 - Regression(r240562) Audio sometimes keeps playing in previous process after a process-swap
Summary: Regression(r240562) Audio sometimes keeps playing in previous process after a...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on: 196784
Blocks: 193716
  Show dependency treegraph
 
Reported: 2019-04-10 10:19 PDT by Chris Dumez
Modified: 2019-05-10 15:31 PDT (History)
6 users (show)

See Also:


Attachments
Patch (9.47 KB, patch)
2019-04-10 10:32 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (9.50 KB, patch)
2019-04-10 11:06 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 2019-04-10 10:19:02 PDT
Audio sometimes keeps playing in previous process after a process-swap since r240562.
Comment 1 Chris Dumez 2019-04-10 10:19:39 PDT
<rdar://problem/49460572>
Comment 2 Chris Dumez 2019-04-10 10:32:00 PDT
Created attachment 367136 [details]
Patch
Comment 3 Chris Dumez 2019-04-10 11:06:17 PDT
Created attachment 367141 [details]
Patch
Comment 4 Alex Christensen 2019-04-10 12:54:05 PDT
Comment on attachment 367141 [details]
Patch

This looks good.  I lament the lack of tests.
Comment 5 Chris Dumez 2019-04-10 12:55:05 PDT
(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.
Comment 6 Chris Dumez 2019-04-10 12:56:48 PDT
Comment on attachment 367141 [details]
Patch

Clearing flags on attachment: 367141

Committed r244144: <https://trac.webkit.org/changeset/244144>
Comment 7 Chris Dumez 2019-04-10 12:56:50 PDT
All reviewed patches have been landed.  Closing bug.
Comment 8 Ryan Haddad 2019-04-10 14:28:01 PDT
(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
Comment 9 Chris Dumez 2019-04-10 14:28:57 PDT
(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.
Comment 10 Chris Dumez 2019-04-10 14:30:02 PDT
(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.
Comment 11 Chris Dumez 2019-04-10 16:51:28 PDT
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.
Comment 12 Chris Dumez 2019-04-11 06:31:39 PDT
Follow up fix landed in https://trac.webkit.org/changeset/244186