Bug 66464 - REGRESSION (r92231): Apple campus proposal PDF doesn't display in Safari
Summary: REGRESSION (r92231): Apple campus proposal PDF doesn't display in Safari
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.7
: P2 Normal
Assignee: Michael Saboff
URL: http://www.cupertino.org/inc/pdf/appl...
Keywords: InRadar, PlatformOnly, Regression
Depends on:
Blocks:
 
Reported: 2011-08-18 06:50 PDT by Adam Roben (:aroben)
Modified: 2011-08-24 15:14 PDT (History)
6 users (show)

See Also:


Attachments
Proposed patch (4.45 KB, patch)
2011-08-19 14:56 PDT, Michael Saboff
no flags Details | Formatted Diff | Diff
Patch updated based on comment 9 (3.39 KB, patch)
2011-08-21 21:42 PDT, Michael Saboff
andersca: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Roben (:aroben) 2011-08-18 06:50:54 PDT
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.
Comment 1 Radar WebKit Bug Importer 2011-08-18 06:51:12 PDT
<rdar://problem/9975906>
Comment 2 Alexey Proskuryakov 2011-08-18 11:02:30 PDT
I cannot reproduce this problem with Safari 5.1 on Mac OS X 10.7.
Comment 3 Adam Roben (:aroben) 2011-08-18 12:20:23 PDT
Looks like this regression is caused by some non-WebKit component. We'll track this in Radar.
Comment 4 Adam Roben (:aroben) 2011-08-19 04:58:50 PDT
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>.
Comment 5 Adam Roben (:aroben) 2011-08-19 04:59:59 PDT
<rdar://problem/9971220>
Comment 6 Michael Saboff 2011-08-19 13:53:09 PDT
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).
Comment 7 Anders Carlsson 2011-08-19 13:56:10 PDT
One thing we could do is to allocate ArgumentEncoders with regular malloc and change the physical copy back to a virtual copy...
Comment 8 Michael Saboff 2011-08-19 14:56:31 PDT
Created attachment 104575 [details]
Proposed patch
Comment 9 Anders Carlsson 2011-08-19 15:03:00 PDT
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.
Comment 10 Michael Saboff 2011-08-21 21:42:57 PDT
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.
Comment 11 Adam Roben (:aroben) 2011-08-22 05:56:25 PDT
Comment on attachment 104638 [details]
Patch updated based on comment 9

Is it possible to write a test for this? (Perhaps using TestWebKitAPI?)
Comment 12 Michael Saboff 2011-08-22 11:28:57 PDT
(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.
Comment 13 Anders Carlsson 2011-08-22 11:31:01 PDT
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.
Comment 14 Adam Roben (:aroben) 2011-08-22 11:51:32 PDT
(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.
Comment 15 Michael Saboff 2011-08-22 14:47:03 PDT
Committed r93546: <http://trac.webkit.org/changeset/93546>
Comment 16 Adam Roben (:aroben) 2011-08-22 16:56:53 PDT
Michael, do you plan to write a regression test for this?
Comment 17 Michael Saboff 2011-08-24 15:14:28 PDT
(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.