Bug 237278 – Remove release assert when UI process is blocking IOSurface IOKit access

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

[Per Arne Vollan]

Remove release assert in IOSurface::bytesPerRowAlignment() when UI process is blocking IOSurface IOKit access.

[Per Arne Vollan]

Created attachment 453405 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 453405 [details]
Patch

I'm not sure why we'd special-case this one, and not the other places where we make IOKit calls which need to be removed before we can enable blocking?

[Per Arne Vollan]

(In reply to Simon Fraser (smfr) from comment #2)
> Comment on attachment 453405 [details]
> Patch
> 
> I'm not sure why we'd special-case this one, and not the other places where
> we make IOKit calls which need to be removed before we can enable blocking?

This IOKit call only takes place in the UI process. We're brokering the value to the WebContent process.

Thanks for reviewing!

[Darin Adler]

Comment on attachment 453405 [details]
Patch

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

> Source/WebCore/platform/graphics/cocoa/IOSurface.mm:299
> +            // 64 bytes is currently the alignment on all platforms.
> +            alignment = 64;

Why not just return a constant? We don’t need to call IOSurface just to feel good.

[Per Arne Vollan]

(In reply to Darin Adler from comment #4)
> Comment on attachment 453405 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=453405&action=review
> 
> > Source/WebCore/platform/graphics/cocoa/IOSurface.mm:299
> > +            // 64 bytes is currently the alignment on all platforms.
> > +            alignment = 64;
> 
> Why not just return a constant? We don’t need to call IOSurface just to feel
> good.

That is a good point. I kept the IOSurface call in case the constant should change at some point.

Thanks for reviewing!

[EWS]

Committed r290618 (247891@main): <https://commits.webkit.org/247891@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 453405 [details].

[Radar WebKit Bug Importer]

<rdar://problem/89585043>

[Geoffrey Garen]

Comment on attachment 453405 [details]
Patch

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

> Source/WebCore/platform/graphics/cocoa/IOSurface.mm:297
> +            RELEASE_LOG_FAULT(Layers, "Sandbox does not allow IOSurface IOKit access.");

RELEASE_LOG_FAULT produces a CrashTracer report. Does this code path execute frequently? If so, it will become our top CrashTracer report (even if the user does not experience a process termination). Do we have other options for gathering whatever information we're trying to gather here?

[Per Arne Vollan]

(In reply to Geoffrey Garen from comment #8)
> Comment on attachment 453405 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=453405&action=review
> 
> > Source/WebCore/platform/graphics/cocoa/IOSurface.mm:297
> > +            RELEASE_LOG_FAULT(Layers, "Sandbox does not allow IOSurface IOKit access.");
> 
> RELEASE_LOG_FAULT produces a CrashTracer report. Does this code path execute
> frequently? If so, it will become our top CrashTracer report (even if the
> user does not experience a process termination). Do we have other options
> for gathering whatever information we're trying to gather here?

That is a very good point. The code path is expected to execute very infrequently, although I still think it makes sense to avoid possible crash reports. I will change this to log an error instead.

Thanks for reviewing!

[Per Arne Vollan]

Reopening to attach new patch.

[Per Arne Vollan]

Created attachment 453503 [details]
Patch

[EWS]

Committed r290675 (247946@main): <https://commits.webkit.org/247946@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 453503 [details].