Bug 85275 – REGRESSION(r115654): PDFs come up blank

• Status: RESOLVED
• Resolution: FIXED
• Component: PDF

[Steven Kolins]

It works in other browser/plugin combinations and works in Safari itself. I'm using the Apple plugins only for viewing pdfs.

[Alexey Proskuryakov]

Regressed in <http://trac.webkit.org/changeset/115654> (Move more of committing and starting to write a Document to DocumentLoader).

[Alexey Proskuryakov]

<rdar://problem/11354915>

[Nate Chapin]

(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.

[Alexey Proskuryakov]

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?

[Nate Chapin]

(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.

[Alexey Proskuryakov]

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?

[Nate Chapin]

(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.

[Alexey Proskuryakov]

If it gets complicated, let's just get a fix landed ASAP.

[Nate Chapin]

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.

[Nate Chapin]

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.

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Nate Chapin]

Created attachment 139708 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 139708 [details]
Patch for landing

Clearing flags on attachment: 139708

Committed r115774: <http://trac.webkit.org/changeset/115774>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

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.

[Csaba Osztrogonác]

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?

[Csaba Osztrogonác]

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 ...

[Nate Chapin]

(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.

[Martin Robinson]

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.

[Nate Chapin]

(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?

[Martin Robinson]

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.

[Alexey Proskuryakov]

Bug 85352 tracks regression test failures on Chromium, perhaps that bug can be repurposed for cross-platform.

[Nate Chapin]

Rebaselined:
http://trac.webkit.org/changeset/115864
http://trac.webkit.org/changeset/115876