Bug 85275 - REGRESSION(r115654): PDFs come up blank
: REGRESSION(r115654): PDFs come up blank
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: PDF
: 528+ (Nightly build)
: Macintosh Intel Mac OS X 10.7
: P1 Critical
Assigned To: Nate Chapin
http://bahai-library.com/pdf/2012_05/...
: InRadar, Regression
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-01 04:53 PDT by Steven Kolins
Modified: 2012-05-02 14:08 PDT (History)
8 users (show)

See Also:


Attachments
Patch (without test) (2.66 KB, patch)
2012-05-01 13:11 PDT, Nate Chapin
no flags Details | Formatted Diff | Diff
Patch (with test) (5.39 KB, patch)
2012-05-01 15:34 PDT, Nate Chapin
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-04 (5.79 MB, application/zip)
2012-05-01 16:15 PDT, WebKit Review Bot
no flags Details
Patch for landing (6.38 KB, patch)
2012-05-01 16:37 PDT, Nate Chapin
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Steven Kolins 2012-05-01 04:53:38 PDT
It works in other browser/plugin combinations and works in Safari itself. I'm using the Apple plugins only for viewing pdfs.
Comment 1 Alexey Proskuryakov 2012-05-01 09:49:21 PDT
Regressed in <http://trac.webkit.org/changeset/115654> (Move more of committing and starting to write a Document to DocumentLoader).
Comment 2 Alexey Proskuryakov 2012-05-01 09:52:23 PDT
<rdar://problem/11354915>
Comment 3 Nate Chapin 2012-05-01 10:56:29 PDT
(In reply to comment #2)
> <rdar://problem/11354915>

I have a probable fix, just running layout tests to verify.

Is there a good way to add a test for this? I'm not familiar with testing the Safari pdf viewer.
Comment 4 Alexey Proskuryakov 2012-05-01 11:25:40 PDT
The code is all in WebKit, so this should be testable, but we don't have any tests AFAICT.

Perhaps this is detectable with loader callbacks?
Comment 5 Nate Chapin 2012-05-01 11:34:38 PDT
(In reply to comment #4)
> The code is all in WebKit, so this should be testable, but we don't have any tests AFAICT.
> 
> Perhaps this is detectable with loader callbacks?

It would be, though for it to fail as the code stands currently, I need a resource for which m_client->hasHTMLView() returns false.
Comment 6 Alexey Proskuryakov 2012-05-01 11:42:35 PDT
Navigating to a PDF from an original html test would do that, although I'm not sure how to notifyDone() in this case. Maybe from a separate window?
Comment 7 Nate Chapin 2012-05-01 11:46:24 PDT
(In reply to comment #6)
> Navigating to a PDF from an original html test would do that, although I'm not sure how to notifyDone() in this case. Maybe from a separate window?

Ok, I'll look into that as soon as I can. Thanks for the ideas.
Comment 8 Alexey Proskuryakov 2012-05-01 12:59:35 PDT
If it gets complicated, let's just get a fix landed ASAP.
Comment 9 Nate Chapin 2012-05-01 13:11:31 PDT
Created attachment 139664 [details]
Patch (without test)

I'll keep working on a test, but given the urgency, I want to get this on EWS and make sure the code change looks ok.
Comment 10 Nate Chapin 2012-05-01 15:34:00 PDT
Created attachment 139694 [details]
Patch (with test)

The test is actually super simple.  It doesn't assert without the patch (like the nightly does), but you do see 2 didCommitLoadForFrame calls for the iframe.
Comment 11 WebKit Review Bot 2012-05-01 16:15:15 PDT
Comment on attachment 139694 [details]
Patch (with test)

Attachment 139694 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12587680

New failing tests:
http/tests/loading/pdf-commit-load-callbacks.html
Comment 12 WebKit Review Bot 2012-05-01 16:15:21 PDT
Created attachment 139700 [details]
Archive of layout-test-results from ec2-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment 13 Nate Chapin 2012-05-01 16:37:15 PDT
Created attachment 139708 [details]
Patch for landing
Comment 14 WebKit Review Bot 2012-05-01 18:22:27 PDT
Comment on attachment 139708 [details]
Patch for landing

Clearing flags on attachment: 139708

Committed r115774: <http://trac.webkit.org/changeset/115774>
Comment 15 WebKit Review Bot 2012-05-01 18:22:41 PDT
All reviewed patches have been landed.  Closing bug.
Comment 16 Csaba Osztrogonác 2012-05-02 00:54:26 PDT
Why are the platform independent and the chromium specific expected results different? Which is the correct? The committed result for chromium matches the actual result on Qt port.
Comment 17 Csaba Osztrogonác 2012-05-02 00:59:47 PDT
Chromium's expected result:
main frame - didStartProvisionalLoadForFrame
main frame - didCommitLoadForFrame
frame "<!--framePath //<!--frame0-->-->" - didStartProvisionalLoadForFrame
main frame - didFinishDocumentLoadForFrame
main frame - didHandleOnloadEventsForFrame
frame "<!--framePath //<!--frame0-->-->" - didFailProvisionalLoadWithError
main frame - didFinishLoadForFrame

"plaform independent" expected result (but different on each platform):
main frame - didStartProvisionalLoadForFrame
main frame - didCommitLoadForFrame
frame "<!--framePath //<!--frame0-->-->" - didStartProvisionalLoadForFrame
main frame - didFinishDocumentLoadForFrame
frame "<!--framePath //<!--frame0-->-->" - didCommitLoadForFrame
frame "<!--framePath //<!--frame0-->-->" - didFinishDocumentLoadForFrame
frame "<!--framePath //<!--frame0-->-->" - didHandleOnloadEventsForFrame
main frame - didHandleOnloadEventsForFrame
frame "<!--framePath //<!--frame0-->-->" - didFinishLoadForFrame
main frame - didFinishLoadForFrame

Qt's actual result:
main frame - didStartProvisionalLoadForFrame
main frame - didCommitLoadForFrame
frame "<!--framePath //<!--frame0-->-->" - didStartProvisionalLoadForFrame
main frame - didFinishDocumentLoadForFrame
main frame - didHandleOnloadEventsForFrame
frame "<!--framePath //<!--frame0-->-->" - didFailProvisionalLoadWithError
main frame - didFinishLoadForFrame

GTK's actual result:
main frame - didStartProvisionalLoadForFrame
main frame - didCommitLoadForFrame
frame "<!--framePath //<!--frame0-->-->" - didStartProvisionalLoadForFrame
main frame - didFinishDocumentLoadForFrame
main frame - didHandleOnloadEventsForFrame
main frame - didFinishLoadForFrame

Could you check which results are correct or not?
Comment 18 Csaba Osztrogonác 2012-05-02 05:08:19 PDT
The test is skipped on Qt - http://trac.webkit.org/changeset/115816
Please unskip it with the proper fix.

I don't want to confuse anybody, so I didn't reopen the bug, because it is fixed, apart from the test fails on more platforms ...
Comment 19 Nate Chapin 2012-05-02 09:24:20 PDT
(In reply to comment #18)
> The test is skipped on Qt - http://trac.webkit.org/changeset/115816
> Please unskip it with the proper fix.
> 
> I don't want to confuse anybody, so I didn't reopen the bug, because it is fixed, apart from the test fails on more platforms ...

GTK's is the only one that doesn't immediately make sense to me. I'd expect a port to pass and finish the load or fail a provisional load, but GTK is doing neither.
Comment 20 Martin Robinson 2012-05-02 09:34:58 PDT
GTK+'s DRT does not seem to support dumping provisional load failures. On the other hand, it should at least print didFailLoadingWithError, based on my reading of the code.
Comment 21 Nate Chapin 2012-05-02 09:40:49 PDT
(In reply to comment #20)
> GTK+'s DRT does not seem to support dumping provisional load failures. On the other hand, it should at least print didFailLoadingWithError, based on my reading of the code.

I wouldn't think didFailLoadingWithError is expected, but I may be missing something. Specifically, I can't find an example of a didFailProvisionalLoadWithError that also fires didFailLoadingWithError.

Is there a reason GTK would substitute didFailLoadingWithError for didFailProvisionalLoadWithError?
Comment 22 Martin Robinson 2012-05-02 09:44:21 PDT
The WebKit1 GTK+ port sends the same signal to the client for both errors.  It's up to the client to sort out what is a provisional failure and what is a failure after the provisional phase. The GTK+ DRT doesn't do this at the moment, it seems.
Comment 23 Alexey Proskuryakov 2012-05-02 09:53:59 PDT
Bug 85352 tracks regression test failures on Chromium, perhaps that bug can be repurposed for cross-platform.