Bug 85275 - REGRESSION(r115654): PDFs come up blank
: REGRESSION(r115654): PDFs come up blank
Status: RESOLVED FIXED
: WebKit
PDF
: 528+ (Nightly build)
: Macintosh Intel Mac OS X 10.7
: P1 Critical
Assigned To:
: http://bahai-library.com/pdf/2012_05/...
: InRadar, Regression
:
:
  Show dependency treegraph
 
Reported: 2012-05-01 04:53 PST by
Modified: 2012-05-02 14:08 PST (History)


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


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2012-05-01 04:53:38 PST
It works in other browser/plugin combinations and works in Safari itself. I'm using the Apple plugins only for viewing pdfs.
------- Comment #1 From 2012-05-01 09:49:21 PST -------
Regressed in <http://trac.webkit.org/changeset/115654> (Move more of committing and starting to write a Document to DocumentLoader).
------- Comment #2 From 2012-05-01 09:52:23 PST -------
<rdar://problem/11354915>
------- Comment #3 From 2012-05-01 10:56:29 PST -------
(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 From 2012-05-01 11:25:40 PST -------
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 From 2012-05-01 11:34:38 PST -------
(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 From 2012-05-01 11:42:35 PST -------
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 From 2012-05-01 11:46:24 PST -------
(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 From 2012-05-01 12:59:35 PST -------
If it gets complicated, let's just get a fix landed ASAP.
------- Comment #9 From 2012-05-01 13:11:31 PST -------
Created an attachment (id=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 From 2012-05-01 15:34:00 PST -------
Created an attachment (id=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 From 2012-05-01 16:15:15 PST -------
(From update of attachment 139694 [details])
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 From 2012-05-01 16:15:21 PST -------
Created an attachment (id=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 From 2012-05-01 16:37:15 PST -------
Created an attachment (id=139708) [details]
Patch for landing
------- Comment #14 From 2012-05-01 18:22:27 PST -------
(From update of attachment 139708 [details])
Clearing flags on attachment: 139708

Committed r115774: <http://trac.webkit.org/changeset/115774>
------- Comment #15 From 2012-05-01 18:22:41 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #16 From 2012-05-02 00:54:26 PST -------
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 From 2012-05-02 00:59:47 PST -------
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 From 2012-05-02 05:08:19 PST -------
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 From 2012-05-02 09:24:20 PST -------
(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 From 2012-05-02 09:34:58 PST -------
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 From 2012-05-02 09:40:49 PST -------
(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 From 2012-05-02 09:44:21 PST -------
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 From 2012-05-02 09:53:59 PST -------
Bug 85352 tracks regression test failures on Chromium, perhaps that bug can be repurposed for cross-platform.
------- Comment #24 From 2012-05-02 14:08:48 PST -------
Rebaselined:
http://trac.webkit.org/changeset/115864
http://trac.webkit.org/changeset/115876