Bug 7145

Created attachment 6357 [details] Patch v1 First pass at a patch. I'm pretty sure I'll need a different NSError, but this code at least shows the URL in the Activity Window. Also, I wasn't sure whether the NSError needed to be released. When I tried to release it, the Activity Window would either cause a Bus Error or just not show up (with an error logged to the Terminal window where I started WebKit+Safari). See Attachment 6314 [details] from Bug 3527 for an invalid PostScript file to test with.

Comment on attachment 6357 [details] Patch v1 Seems like the concept here is OK, but the error message is too vague. Also not sure if just calling the delegate is enough. We might also have to put the loader into the right "state".

Comment on attachment 6357 [details] Patch v1 This looks like the right approach. The NSError doesn't need to be released, because it is returned from _webKitErrorWithDomain:code:URL: as an autoreleased object. WebKitErrorCannotShowURL seems like the best choice of the existing WebKit errors, but I agree that it's not quite appropriate. We should invent a new error code just for this case. We can't modify WebKitErrors.h without API approval, so a new error would have to be added to WebKitErrorsPrivate.h. Please add a comment explaining under what circumstances this conversion could fail. Also, please attach at least one test file to this bug that causes the error to occur.

Created attachment 6376 [details] Test case (invalid PS file)

(In reply to comment #3) > Please add a comment explaining under what circumstances this conversion could > fail. Also, please attach at least one test file to this bug that causes the > error to occur. The circumstances under which this could occur are if CGPSConverterConvert() is passed an NSData object that is not a valid PostScript file. I attached such a sample (basically the first few lines of a valid PostScript file with the rest of the content truncated) as Attachment 6376 [details]. That method call simply returns a boolean value: true if the conversion succeeded or false if it did not. Note that the CGPSConverterCallbacks structure has a callback method for passing messages about the conversion, e.g., if it fails, back to the caller. I have not tried implementing this yet to see what kind of message is passed back for the invalid PostScript file I attached. I will do that next. http://developer.apple.com/documentation/GraphicsImaging/Conceptual/drawingwithquartz2d/dq_ps_convert/chapter_16_section_2.html

I wired up a callback method to receive messages, and this is what was sent back for Attachment 6376 [details]: %%[ Warning: Empty job. No PDF file produced. ] %% I'm not sure if these types of errors would be suitable to show the user directly, though.

In another case, I tried renaming 'tiger.ps.gz' (a gzipped version of tiger.ps) to 'tiger.ps' and opening it from the filesystem, and got these callback messages: %%[ Error: undefined; OffendingCommand: ]%% %%[ Flushing: rest of job (to end-of-file) will be ignored ]%% %%[ Warning: PostScript error. No PDF file produced. ] %% Then I took the contents of an existing PDF file and copied them to a file ending in ".ps" (which the Finder happily marked as a PostScript file): $ cat payroll.pdf > foo.ps When I opened this file in Safari, this came out of the message callback method (there were three messages sent; each new message starts with '%%'): %%[ Error: undefined; OffendingCommand: obj ]%% Stack: 0 173 %%[ Flushing: rest of job (to end-of-file) will be ignored ]%% %%[ Warning: PostScript error. No PDF file produced. ] %%

Created attachment 6381 [details] Patch v2 Version 2 of the patch. This one adds a new error code and description. Note that I filed bug 7171 while working on this new patch. > Please add a comment explaining under what circumstances this conversion could > fail. BTW, did you mean a code comment or a Bugzilla comment? I assumed a Bugzilla comment given the context of wanting a sample PostScript file that would cause a failure.

Comment on attachment 6381 [details] Patch v2 I actually meant a code comment, but I'm grateful for the detailed explanation in the bug. Now that there's a WebKit error with a nice precise name, I don't think a code comment is necessary anymore. In the block where you set up the error, there's no reason to have a local variable for webView. Since you've introduced a new localizable string, you need to run the script update-webkit-localizable-strings (in WebKitTools/Scripts/) to update the Localizable.strings file. Otherwise the new string won't appear in the user interface (or be localizable). This makes me wonder how you tested the new error -- did you look in Safari's Activity window to see if the error string appeared? I would expect it not to since Localizable.strings wasn't updated.

(In reply to comment #9) > Since you've introduced a new localizable string, you need to run the script > update-webkit-localizable-strings (in WebKitTools/Scripts/) to update the > Localizable.strings file. Otherwise the new string won't appear in the user > interface (or be localizable). Sorry, I wasn't aware of this step for either this bug or Bug 7171. Is there some documentation that I failed to read in the project? (Not that I've read anything other than ChangeLogs and code, but if there is documentation to read, I'd like to know about it!) > This makes me wonder how you tested the new > error -- did you look in Safari's Activity window to see if the error string > appeared? I would expect it not to since Localizable.strings wasn't updated. Well no error message ever showed up in the Activity window under the URL and the file size when I used WebKitErrorCannotShowURL in Patch v1 (Attachment 6357 [details]), so I wasn't sure whether changing to a new error code would change that or not. (I expected it to, but I wasn't too surprised when it didn't.) I'll try the testcase for Bug 7176 to see if I get an error message in the Activity window. (I'll also compare the results to Safari in 10.4.4 to see if I'm missing anything.)

There is some documentation at http://webkit.opendarwin.org/, but I looked around and didn't see any documentation for update-webkit-localizable-strings. So it's no surprise that you didn't find it, and I shouldn't expect you to run it. Hopefully that will be documented on the site soon, but for now don't worry about it. I'll check the patch in when it's ready and make sure the localized strings are updated. I am surprised that no error text appears in the Activity window though, especially when you used WebKitErrorCannotShowURL. I'll try your patch and debug this on Safari to see what's going on; I'll report back here.

I should also clarify that you won't see any error text in the Activity window for the test case in bug 7176 because Safari deliberately special-cases that error for the reasons explained in that bug.

Hmm, I seem to be missing something. Running with the latest WebKit, when I use Safari to try to view this invalid.ps file, either as a local or as a remote file, Preview takes over and puts up an alert about the failed conversion. Note that this is without applying this patch, but my WebKit does include the postscript-to-PDF conversion from bug 3527. I wanted to check the behavior with and without the patch. How do I make WebKit/Safari handle this PostScript file, rather than Preview?

(In reply to comment #13) > How do I make WebKit/Safari handle this PostScript file, rather than Preview? PostScript files take the same path as PDF files (with the exception of the conversion process) when opened in Safari. Do plain PDF files open in Preview as well when you access them via Safari? If so, you probably have the WebKitOmitPDFSupport preference set to "1" or "YES": defaults read com.apple.Safari WebKitOmitPDFSupport

Nope, that's not it. I don't have that default set, and Safari does display my PDF files. As a local file, invalid.ps isn't selectable in the Open File dialog, since Safari hasn't (yet) been taught that it knows about this file type. I put invalid.ps onto a remote server and tried to open it from there; it was downloaded and handed off to Preview, rather than opened inline. Are you doing something different?

(In reply to comment #15) > As a local file, invalid.ps isn't selectable in the Open File dialog, since > Safari hasn't (yet) been taught that it knows about this file type. Using nightly WebKit-SVN-r12751.dmg, I am able to select invalid.ps from an Open File dialog and it opens in Safari. > I put invalid.ps onto a remote server and tried to open it from there; it was > downloaded and handed off to Preview, rather than opened inline. Using the same nightly build, if I click on the attachment link in this bug, the invalid.ps file opens in Safari. > Are you doing something different? Not that I am aware of. Regarding the error message, I tried running update-webkit-localizable-strings and rebuilding WebKit locally, but when invalid.ps opens in Safari, I do NOT see any error message in the Activity Window. Is the message in Patch v2 too long for it to be displayed? (In reply to comment #9) > In the block where you set up the error, there's no reason to have a local > variable for webView. The "webView" variable is used twice: Once to get the delegate and once to pass the webView back into the delegate method. Are you saying I don't need to get the delegate first and just call _didFailLoadWithError:forFrame directly on the view instead?

(In reply to comment #16) > The "webView" variable is used twice: Once to get the delegate and once to pass > the webView back into the delegate method. Are you saying I don't need to get > the delegate first and just call _didFailLoadWithError:forFrame directly on the > view instead? That won't work since it's a private method.

(In reply to comment #16) > Regarding the error message, I tried running update-webkit-localizable-strings > and rebuilding WebKit locally, but when invalid.ps opens in Safari, I do NOT > see any error message in the Activity Window. Is the message in Patch v2 too > long for it to be displayed? Okay, I finally saw some "other" error messages today in the activity window, like "cancelled", "can't find host", and "lost network connection". (Yes, I'm not happy with the number of packets that my ISP is currently dropping.) What happens with an invalid PS file is that the size of the file turns red in the "Status" column ("0.3 KB") instead of showing the actual message. Does this make sense?

>The "webView" variable is used twice: Once to get the delegate and once to pass >the webView back into the delegate method. I had overlooked the fact that the local variable webView is used twice, so no problem there. >What happens with an invalid PS file is that the size of the file turns red in >the "Status" column ("0.3 KB") instead of showing the actual message. Does >this make sense? I understand what you're saying, but it's not the expected/desired behavior. The file size should be replaced by the error message if there's an error. This seems likely to be a Safari bug rather than a WebKit bug, but I still can't reproduce because when I try to load the file Invalid.ps Preview takes over. I wish I knew why this behavior is different for the two of us. Maybe someone else could try this and report what happens to them?

When I load the PS file with the latest nightly it tries to load inside Safari's main window. If I do `defaults write com.apple.Safari WebKitOmitPDFSupport -bool YES` and restart the nightly the PS file then loads in Preview as expected. Perhaps you have that default enabled John?

I definitely do not have that default set. PDF files are loaded in Safari just fine. But .ps files (or at least the invalid.ps file attached to this bug) are not. Hmm.

(In reply to comment #21) > I definitely do not have that default set. PDF files are loaded in Safari just > fine. But .ps files (or at least the invalid.ps file attached to this bug) are > not. Hmm. Do you want me to gather results from more testers, John? I thought about posting this request to the webkit-dev mailing list, but decided that would be overkill. Are you using a production release version of Mac OS X, or is it something different? Is there another system you can try this on besides your own?

OK, my computer seems to have finally regained its senses. I have no idea what changed between all of my earlier attempts and now, but I can now select the invalid.ps file locally from the Open dialog of Safari. I can also (failed-)load it from a remote location in Safari. I can't explain why I was getting different behavior before, but let's just hope it was some sort of user error on my part. Anyway, now that I've gotten this far, I will apply this patch and see what's going on in the Activity window.

I applied patch v2 and ran it under a debug build of Safari. It hits an assertion there, which shows a flaw in the patch. (The incorrect appearance seen in the Activity window with a size shown in red is probably a side-effect of this flaw, but I didn't investigate that exact issue.) The flaw is that this patch sends -webView:didFailLoadWithError:forFrame: to the delegate within finishedLoadingWithDataSource:, but at that point the loader code in WebMainResourceLoader believes that the load has completed successfully. Shortly after the delegate gets the didFailLoadWithError: callback, it then receives a didFinishLoadForFrame: callback via the normal mechanism. the WebKit API promises to send one or the other of these two callbacks, but not both for the same load. Given that the PDF code can't tell if there was an error until the data has been completely loaded, it seems like fixing this would require rearchitecting the loader to some extent to allow the WebDocumentRepresentation to report errors back to the loader after the load has finished. An alternative approach (perhaps what Darin was thinking about originally) would be to invent a new delegate callback to be used for this kind of post-complete-load error. This would require new code in clients (inc. Safari) to have any effect. Yet another approach would be to create a PDF document on the fly containing a localized string such as "Couldn't convert the PostScript file to a PDF file." (that happens to be the exact string used by Preview). Then in -[WebPDFRepresentation finishedLoadingWithDataSource:], instead of bailing out early in this error case, you'd use this "error PDF" for the PDFDocument that you load in the PDFSubview. If there is such a thing as a zero-length data non-error case, you'd want to distinguish that from the zero-length error case, perhaps with a different message ("This document is empty.", perhaps). This 3rd alternative is attractive because it's self-contained in the WebPDFRepresentation code, and would work well for existing WebKit clients. If you're interested in pursuing one of the other alternatives, you'll probably want to discuss it in more detail with either Darin Adler or Maciej Stachowiak, both of whom know more about this loader architecture than I do.

Reassigning to default owner of this component since I don't have time to work on it now, and my focus is elsewhere (web archive formats). Per Comment 24, I think this is the best approach: > An alternative approach (perhaps what Darin was thinking about originally) > would be to invent a new delegate callback to be used for this kind of > post-complete-load error. This would require new code in clients (inc. Safari) > to have any effect. Displaying a dynamically-composed PDF to announce the error would be user-friendly for interactive users, but not if someone was trying to use WebKit programmatically. In that case they'd pull their hair out trying to come up with a work-around since they'd see a valid PS file be displayed without knowing whether an error had happened or not.