Bug 66464 – REGRESSION (r92231): Apple campus proposal PDF doesn't display in Safari

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

[Adam Roben (:aroben)]

To reproduce:

1. Go to http://www.cupertino.org/inc/pdf/apple/intro.pdf

All you get is a gray WKView area. The PDF displays just fine in WebKit1, and in Preview.

[Radar WebKit Bug Importer]

<rdar://problem/9975906>

[Alexey Proskuryakov]

I cannot reproduce this problem with Safari 5.1 on Mac OS X 10.7.

[Adam Roben (:aroben)]

Looks like this regression is caused by some non-WebKit component. We'll track this in Radar.

[Adam Roben (:aroben)]

Turns out this is a WebKit regression after all. This change is to blame: <http://trac.webkit.org/changeset/92231/trunk/Source/WebKit2/Platform/CoreIPC/mac/ConnectionMac.cpp>.

[Adam Roben (:aroben)]

<rdar://problem/9971220>

[Michael Saboff]

From Mark Rowe:
After the change in r92231 the call to mach_msg is failing with MACH_MSG_VM_KERNEL, indicating "Kernel resource shortage handling out-of-line memory", when attempting to send the message containing the PDF data from the web process over to the UI process (below WebFrameLoaderClient::finishedLoading).

[Anders Carlsson]

One thing we could do is to allocate ArgumentEncoders with regular malloc and change the physical copy back to a virtual copy...

[Michael Saboff]

Created attachment 104575 [details]
Proposed patch

[Anders Carlsson]

Comment on attachment 104575 [details]
Proposed patch

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

> Source/WebKit2/Platform/CoreIPC/mac/ConnectionMac.cpp:162
> +        allocatedBuffer = reinterpret_cast<char*>(mmap(0, messageSize, PROT_READ | PROT_WRITE, MAP_ANON | MAP_PRIVATE, -1, 0));
> +        ASSERT(allocatedBuffer != MAP_FAILED);
> +        buffer = allocatedBuffer;

I think you could do a vm_allocate here instead of an mmap - not sure if it matters much though.

> Source/WebKit2/Platform/CoreIPC/mac/ConnectionMac.cpp:211
> +    memcpy(messageData, arguments->buffer(), arguments->bufferSize());

It's sad that we always have to copy the data now, even if the data is OOL. That means that we'd have to memcpy ~15M for the Campus PDF.

Making ArgumentEncoder use malloc/free and change back the MACH_MSG_PHYSICAL_COPY to MACH_MSG_VIRTUAL_COPY would fix that bug.

[Michael Saboff]

Created attachment 104638 [details]
Patch updated based on comment 9

Changed to use system malloc in ArgumentEncoder and returned to using MACH_MSG_VIRTUAL_COPY with OOL mach_msg.

Verified no increase in memory usage nor page load time.

[Adam Roben (:aroben)]

Comment on attachment 104638 [details]
Patch updated based on comment 9

Is it possible to write a test for this? (Perhaps using TestWebKitAPI?)

[Michael Saboff]

(In reply to comment #11)
> (From update of attachment 104638 [details])
> Is it possible to write a test for this? (Perhaps using TestWebKitAPI?)

We can add a test.  However, the proposed change doesn't change logic, just the source of the memory and the semantics of the mach_msg.  Seems to be heavy weight given this change.

[Anders Carlsson]

Comment on attachment 104638 [details]
Patch updated based on comment 9

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

> Source/WebKit2/Platform/CoreIPC/ArgumentEncoder.cpp:74
> +        // fastMalloc using MADV_FREE_REUSABE doesn't work with

Typo, MADV_FREE_REUSABLE.

[Adam Roben (:aroben)]

(In reply to comment #12)
> (In reply to comment #11)
> > (From update of attachment 104638 [details] [details])
> > Is it possible to write a test for this? (Perhaps using TestWebKitAPI?)
> 
> We can add a test.  However, the proposed change doesn't change logic, just the source of the memory and the semantics of the mach_msg.  Seems to be heavy weight given this change.

We don't typically decide whether to write a test or not based on the size of the code change. We write a test if the code change results in an observable change in behavior, which this one does.

[Michael Saboff]

Committed r93546: <http://trac.webkit.org/changeset/93546>

[Adam Roben (:aroben)]

Michael, do you plan to write a regression test for this?

[Michael Saboff]

(In reply to comment #16)
> Michael, do you plan to write a regression test for this?

At this point we don't plan on adding a test. The brute force regression test is putting the 15MB PDF file in the tree and making sure we can open it.  That was deemed to be too costly.

What would be ideal is to have a set of tests (unit / functional) that test the IOCore functionality including the argument encoder / decoders.  That effort is beyond the scope of this change of restoring the functionality by using a different mallocator.