Bug 187635 – [iOS] [WK1] Crash inside IOSurfacePool::platformGarbageCollectNow() in WebThread

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Ryosuke Niwa]

[CATransaction commit] can end up causing UIKit to do a layout.
We can't do that in WebKit1.

[Ryosuke Niwa]

<rdar://problem/34297065>

[Ryosuke Niwa]

Created attachment 344928 [details]
Fixes the crash

[Tim Horton]

(In reply to Ryosuke Niwa from comment #0)
> [CATransaction commit] can end up causing UIKit to do a layout.
> We can't do that in WebKit1.

Sure we can? We often (and necessarily) commit on the Web Thread (and always have). We depend on being extremely careful that our commits don’t include anything other than our layers to make that OK.

[Ryosuke Niwa]

(In reply to Tim Horton from comment #3)
> (In reply to Ryosuke Niwa from comment #0)
> > [CATransaction commit] can end up causing UIKit to do a layout.
> > We can't do that in WebKit1.
> 
> Sure we can? We often (and necessarily) commit on the Web Thread (and always
> have). We depend on being extremely careful that our commits don’t include
> anything other than our layers to make that OK.

The problem is that we're not doing that here. I guess I can clarify that in the change log?

[Tim Horton]

(In reply to Ryosuke Niwa from comment #4)
> (In reply to Tim Horton from comment #3)
> > (In reply to Ryosuke Niwa from comment #0)
> > > [CATransaction commit] can end up causing UIKit to do a layout.
> > > We can't do that in WebKit1.
> > 
> > Sure we can? We often (and necessarily) commit on the Web Thread (and always
> > have). We depend on being extremely careful that our commits don’t include
> > anything other than our layers to make that OK.
> 
> The problem is that we're not doing that here. I guess I can clarify that in
> the change log?

I mean, if you have an explanation for why the commits include anything other than our layers, please put it in the ChangeLog! (But... do you? I have seen no indication of that.)

While you’re at it, please also explain why it’s OK that we still do other CA commits on the Web thread (what makes this empty commit different?).

ALL THAT SAID, I agree that this empty commit is not necessary in the context of WebKit1. It’s not necessary in the context of any UI process at all, in fact. So I don’t object to your patch, just the reasoning :P

[Ryosuke Niwa]

(In reply to Tim Horton from comment #5)
> (In reply to Ryosuke Niwa from comment #4)
> > (In reply to Tim Horton from comment #3)
> > > (In reply to Ryosuke Niwa from comment #0)
> > > > [CATransaction commit] can end up causing UIKit to do a layout.
> > > > We can't do that in WebKit1.
> > > 
> > > Sure we can? We often (and necessarily) commit on the Web Thread (and always
> > > have). We depend on being extremely careful that our commits don’t include
> > > anything other than our layers to make that OK.
> > 
> > The problem is that we're not doing that here. I guess I can clarify that in
> > the change log?
> 
> I mean, if you have an explanation for why the commits include anything
> other than our layers, please put it in the ChangeLog! (But... do you? I
> have seen no indication of that.)
> 
> While you’re at it, please also explain why it’s OK that we still do other
> CA commits on the Web thread (what makes this empty commit different?).

Please see the radar since you have access to it.

> ALL THAT SAID, I agree that this empty commit is not necessary in the
> context of WebKit1. It’s not necessary in the context of any UI process at
> all, in fact. So I don’t object to your patch, just the reasoning :P

The begin/commit pair was added to free memory in iOS WebKit2 when it was first brought up. I think it's too risky to change that behavior right now so I'm leaving it intact.

[Tim Horton]

(In reply to Ryosuke Niwa from comment #6)
> (In reply to Tim Horton from comment #5)
> > (In reply to Ryosuke Niwa from comment #4)
> > > (In reply to Tim Horton from comment #3)
> > > > (In reply to Ryosuke Niwa from comment #0)
> > > > > [CATransaction commit] can end up causing UIKit to do a layout.
> > > > > We can't do that in WebKit1.
> > > > 
> > > > Sure we can? We often (and necessarily) commit on the Web Thread (and always
> > > > have). We depend on being extremely careful that our commits don’t include
> > > > anything other than our layers to make that OK.
> > > 
> > > The problem is that we're not doing that here. I guess I can clarify that in
> > > the change log?
> > 
> > I mean, if you have an explanation for why the commits include anything
> > other than our layers, please put it in the ChangeLog! (But... do you? I
> > have seen no indication of that.)
> > 
> > While you’re at it, please also explain why it’s OK that we still do other
> > CA commits on the Web thread (what makes this empty commit different?).
> 
> Please see the radar since you have access to it.

I see no such explanation there.

> > ALL THAT SAID, I agree that this empty commit is not necessary in the
> > context of WebKit1. It’s not necessary in the context of any UI process at
> > all, in fact. So I don’t object to your patch, just the reasoning :P
> 
> The begin/commit pair was added to free memory in iOS WebKit2 when it was
> first brought up. I think it's too risky to change that behavior right now
> so I'm leaving it intact.

Right! That part is fine.

[Simon Fraser (smfr)]

Comment on attachment 344928 [details]
Fixes the crash

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

> Source/WebCore/ChangeLog:13
> +        Don't invoke [CATransaction begin] and [CATransaction commit] pair in WebKit1 since
> +        commiting a transaction can result in a UIKit layout which is not safe in WebThread.
> +
> +        This code was added in r167717 to reduce the memory usage of WebContent process in iOS
> +        but in WebKit1, there will be other code in the app doing CA commits.

This needs a better explanation which I don't have time to write at the moment.

> Source/WebCore/platform/graphics/cocoa/IOSurfacePoolCocoa.mm:40
> +    // FIXME: Check if this dummy begin/commit added by <https://webkit.org/b/132065> / <rdar://problem/16110687> is still needed.

I think we know that it is, so change this comment (to make this code less mysterious) so something like:
// We need to trigger a CA commit in the web process to trigger the release layer-related memory, since the WebProcess doesn't normally do CA commits.

[EWS Watchlist]

Comment on attachment 344928 [details]
Fixes the crash

Attachment 344928 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/8526755

New failing tests:
http/tests/security/canvas-remote-read-remote-video-blocked-no-crossorigin.html

[EWS Watchlist]

Created attachment 344962 [details]
Archive of layout-test-results from ews201 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews201  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Simon Fraser (smfr)]

Comment on attachment 344928 [details]
Fixes the crash

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

>> Source/WebCore/ChangeLog:13
>> +        but in WebKit1, there will be other code in the app doing CA commits.
> 
> This needs a better explanation which I don't have time to write at the moment.

Here's some new wording:

r167717 added code to trigger a CA commit in the web process via platformGarbageCollectNow() in order to free IOSurface-related memory.
However, that code is also running in the web thread in apps using WebKit1, causing unwanted UIView layout on the web thread.

Fix by not triggering this CA commit if it's called on the web thread.

[Ryosuke Niwa]

Committed r233816: <https://trac.webkit.org/changeset/233816>