[iOS] Lock screen controls can fail to play web content

[Jer Noble]

[iOS] Lock screen controls can fail to play web content

[Jer Noble]

<rdar://50920870>

[Jer Noble]

Created attachment 379663 [details]
Patch

[Chris Dumez]

Comment on attachment 379663 [details]
Patch

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

> Source/WebKit/Shared/Cocoa/ProcessTaskStateObserver.h:61
> +        virtual void refClient() = 0;

Should these be private?

> Source/WebKit/Shared/Cocoa/ProcessTaskStateObserver.mm:72
> +    return *new ProcessTaskStateObserver(client);

Missing the adoptRef(), so this will leak.

> Source/WebKit/Shared/Cocoa/ProcessTaskStateObserver.mm:95
> +void ProcessTaskStateObserver::invalidate()

Who is calling this? I could not find a caller.

> Source/WebKit/Shared/Cocoa/ProcessTaskStateObserver.mm:104
> +    return makeRefPtr(m_client);

Note that this is not generally thread safe, what guarantees that the destructor of m_client is not currently running on another thread.

> Source/WebKit/WebProcess/WebProcess.h:455
> +    void derefClient() final { ref(); }

should be deref() :(

[Jer Noble]

(In reply to Chris Dumez from comment #3)
> Comment on attachment 379663 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=379663&action=review
> 
> > Source/WebKit/Shared/Cocoa/ProcessTaskStateObserver.h:61
> > +        virtual void refClient() = 0;
> 
> Should these be private?

Sure!

> > Source/WebKit/Shared/Cocoa/ProcessTaskStateObserver.mm:72
> > +    return *new ProcessTaskStateObserver(client);
> 
> Missing the adoptRef(), so this will leak.

Oof. Will fix.

> > Source/WebKit/Shared/Cocoa/ProcessTaskStateObserver.mm:95
> > +void ProcessTaskStateObserver::invalidate()
> 
> Who is calling this? I could not find a caller.

No one. The only client so far is WebProcess, which is never destroyed, so there's no good place to call invalidate (nor any need).

> > Source/WebKit/Shared/Cocoa/ProcessTaskStateObserver.mm:104
> > +    return makeRefPtr(m_client);
> 
> Note that this is not generally thread safe, what guarantees that the
> destructor of m_client is not currently running on another thread.

The only guarantee is that the client has to call invalidate() before their own destructor is called (because at that point their own refcount will be zero, and you can't ref a zero ref count object). Again, since the only client so far is WebProcess, the client is never destroyed, so NBD, but if any one else adopts this object, they'll need to beware.

> > Source/WebKit/WebProcess/WebProcess.h:455
> > +    void derefClient() final { ref(); }
> 
> should be deref() :(

Gah, copy pasta.

[Jer Noble]

Created attachment 379667 [details]
Patch

[Jer Noble]

Created attachment 379669 [details]
Patch

[Jer Noble]

Created attachment 379685 [details]
Patch

[Chris Dumez]

Comment on attachment 379685 [details]
Patch

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

r=me with changes. Also please fix build failures on EWS.

> Source/WebKit/Shared/Cocoa/ProcessTaskStateObserver.mm:84
> +    invalidate();

Why not simply m_delegate.get().taskStateChangedCallback = nil; ? We don't really need to lock and null out the client.

> Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:318
> +    dispatch_after(dispatch_time(DISPATCH_TIME_NOW, (int64_t)(5 * NSEC_PER_SEC)), dispatch_get_main_queue(), [assertion = WTFMove(uiProcessAssertion)] { });

I seem to remember you need to explicitly call [assertion invalidate]; before deallocating it.

[Jer Noble]

(In reply to Chris Dumez from comment #8)
> Comment on attachment 379685 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=379685&action=review
> 
> r=me with changes. Also please fix build failures on EWS.
> 
> > Source/WebKit/Shared/Cocoa/ProcessTaskStateObserver.mm:84
> > +    invalidate();
> 
> Why not simply m_delegate.get().taskStateChangedCallback = nil; ? We don't
> really need to lock and null out the client.

Probably not given the caller of setTaskState() is also holding a ref to the observer.

> > Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:318
> > +    dispatch_after(dispatch_time(DISPATCH_TIME_NOW, (int64_t)(5 * NSEC_PER_SEC)), dispatch_get_main_queue(), [assertion = WTFMove(uiProcessAssertion)] { });
> 
> I seem to remember you need to explicitly call [assertion invalidate];
> before deallocating it.

Will do.

[Jer Noble]

Created attachment 379692 [details]
Patch for landing

[Jer Noble]

Created attachment 379696 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 379696 [details]
Patch for landing

Clearing flags on attachment: 379696

Committed r250428: <https://trac.webkit.org/changeset/250428>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.