Bug 187635 - [iOS] [WK1] Crash inside IOSurfacePool::platformGarbageCollectNow() in WebThread
Summary: [iOS] [WK1] Crash inside IOSurfacePool::platformGarbageCollectNow() in WebThread
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-07-12 22:37 PDT by Ryosuke Niwa
Modified: 2018-07-13 13:56 PDT (History)
4 users (show)

See Also:


Attachments
Fixes the crash (1.77 KB, patch)
2018-07-12 22:44 PDT, Ryosuke Niwa
simon.fraser: review+
ews-watchlist: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews201 for win-future (12.78 MB, application/zip)
2018-07-13 11:54 PDT, EWS Watchlist
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2018-07-12 22:37:28 PDT
[CATransaction commit] can end up causing UIKit to do a layout.
We can't do that in WebKit1.
Comment 1 Ryosuke Niwa 2018-07-12 22:37:43 PDT
<rdar://problem/34297065>
Comment 2 Ryosuke Niwa 2018-07-12 22:44:44 PDT
Created attachment 344928 [details]
Fixes the crash
Comment 3 Tim Horton 2018-07-12 22:46:03 PDT
(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.
Comment 4 Ryosuke Niwa 2018-07-12 23:19:36 PDT
(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?
Comment 5 Tim Horton 2018-07-12 23:27:26 PDT
(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
Comment 6 Ryosuke Niwa 2018-07-13 11:05:29 PDT
(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.
Comment 7 Tim Horton 2018-07-13 11:14:59 PDT
(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.
Comment 8 Simon Fraser (smfr) 2018-07-13 11:20:20 PDT
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.
Comment 9 EWS Watchlist 2018-07-13 11:54:14 PDT
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
Comment 10 EWS Watchlist 2018-07-13 11:54:26 PDT
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
Comment 11 Simon Fraser (smfr) 2018-07-13 12:49:29 PDT
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.
Comment 12 Ryosuke Niwa 2018-07-13 13:56:38 PDT
Committed r233816: <https://trac.webkit.org/changeset/233816>