Bug 10725 – Image data in from RTFD clipboard data thrown away

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Graham Dennis]

In WebHTMLView's _documentFragmentFromPasteboard:... routine, subresources found in the RTFD part of the clipboard are not added as subresources to the data source so that the data from those resources can be accessed later.

I'm not sure how to construct a testcase for this, as I found this in Sandvox when trying to get the data for an image URL that came from RTFD pasteboard data.

Patch forthcoming.

[Graham Dennis]

Created attachment 10389 [details]
patch

Patch. Uses an enumerator to add each subresource in the subresources array to the data source.

[Darin Adler]

Comment on attachment 10389 [details]
patch

Looks good. Can we create a regression test for this?

[Eric Seidel (no email)]

Comment on attachment 10389 [details]
patch

Tabs.  r-

Otherwise looks fine.  I'm not super familiar with this code though.

[Graham Dennis]

Created attachment 10411 [details]
patch 2

Silly me, I had changed SubEthaEdit to use spaces, but I forgot to change Xcode.

Updated patch.

[Darin Adler]

Comment on attachment 10411 [details]
patch 2

Looks good. Can we create a regression test for this?

[Graham Dennis]

(In reply to comment #5)
> (From update of attachment 10411 [details] [edit])
> Looks good. Can we create a regression test for this?
I'm not sure how. One way I would have thought about doing it would be to actually try and paste RTFD data with an image into a editable div in a regression test, and then it would be checked by a pixel test. However, even with this patch, WebKit still doesn't display images that are pasted in in editable divs. And this is a separate bug.

Any ideas?

[Darin Adler]

Comment on attachment 10411 [details]
patch 2

I haven't reviewed this yet because I want to understand what kind of test we're going to land with it.

Also, the ChangeLog patch looks very short and strange. The entire patch looks wrong like it wasn't made with "svn diff".

[Eric Seidel (no email)]

Comment on attachment 10411 [details]
patch 2

Using the copy/paste functionality inside DumpRenderTree, the dumped -expected.txt should be able to show this change.

The ChangeLog does look busted.  Per darin's comments (and lacking text case or copyright) I'm going to r- this.

Fee free to drop by the channel and perhaps one of us can give you a hand polishing this one off.

[Graham Dennis]

Created attachment 10528 [details]
patch 3

Even when applying the previous patch, images pasted in from RTFD data would not be displayed in WebKit. The reason for this is that WebCore tries to load the applewebdata URLs for the images before the resources have been added to the WebUnarchivingState, and because it is an AppKit-private method that parses the RTFD data and creates the document fragment, we cannot add the resources to the WebUnarchivingState until after WebCore has parsed the document fragment and attempted to load the applewebdata URLs.

My method for fixing this is to delay loading applewebdata URLs while the AppKit method is parsing the RTFD data, and only load these URLs after the WebResources have been added to the WebUnarchivingState.

Fixing this caused an exception in Safari because it uses the _webDataRequestExternalURL of the webdata URL as the key in a dictionary for the activity window. So I changed _webDataRequestExternalURL to not return nil, and to return a more sensible default for displaying in the activity window.

To create a Layout test for this patch I had to load the appropriate data into the NSPasteboard, and the only way I could see how to do this was to use the objective-c plugin for DumpRenderTree and call various methods to load the data and put it into NSPasteboard. However, I needed to call the method -[NSPasteboard declareTypes:owner:] which requires an NSArray argument. Unfortunately, NSArray's are bridged to JS Array's, and when I try and pass a JS Array to the objc method, it receives a WebScriptObject instead of an NSArray, and NSPasteboard doesn't like that. To work around this, I added a convenience method to the DumpRenderTreePasteboard that accepts an NSString instead of an NSArray.

I added two test cases for this patch (one for TIFF pasteboard data and one for RTFD pasteboard data) because I came across bug #10314 by trying to paste TIFF pasteboard data, and the patch to 10314 was committed without a layout test.

[Graham Dennis]

Created attachment 10942 [details]
patch 4

Patch 4. Fixes patch rot.

[Graham Dennis]

Created attachment 10978 [details]
patch 5

I made a mistake in my previous patch where I assumed that the applewebdata url's had non-empty URL paths, in this case we return 'about:blank' for the external web data url as in the earlier code.

[Justin Garcia]

Comment on attachment 10978 [details]
patch 5

Most of this good to me, but I'm not familiar enough with this code to review it.

[Graham Dennis]

Created attachment 11013 [details]
patch 6

Patch 6. Fixes patch rot due to latest changes in the loader code.

Also fixed a problem with -[WebFrame _deliverArchivedResourcesAfterDelay] where it schedules a selector on itself that doesn't exist. (Changed to the correct name)

[Graham Dennis]

Created attachment 11014 [details]
patch 6a

Patch 6a... I forgot to update the pixel results when I fixed patch rot. Fixed.

[Graham Dennis]

Created attachment 11591 [details]
patch 7

Updated patch fixing patch rot.
Works in r17866.

[Graham Dennis]

Created attachment 11592 [details]
patch 8

Patch 8.
Addresses Maciej's comments (via IRC) that we should avoid adding another defer-load mechanism if possible. Now, instead of deferring the NSURLConnection callbacks, the actual loading is deferred.

A reference to WebDataProtocol is still needed to work out when to bypass the cache policy (as the archive is the only way to get the data for WebDataProtocol URLs).

Also, this code removes the need for wkSetNSURLConnectionDefersCallbacks and WKSetNSURLConnectionDefersCallbacks which I think are both referred to in libWebKitSystemInterface.a, however I haven't removed the definitions of these functions just in case...

[Graham Dennis]

Created attachment 11663 [details]
patch 9

This patch removes the code in my previous patch that caused WebKit to refer to WebDataProtocol. I believe this patch now addresses all of Maciej's comments via IRC. There is still one part of the patch that modifies WebDataProtocol.mm, however this isn't needed for the layout tests to pass, but is only needed to prevent Safari crashing.

[Maciej Stachowiak]

Mostly this looks ok. However:

1) It needs to be updated to ToT.

2) Is the WebDataProtocol change definitely safe? There might be code that depends on getting nil in some cases. This change affects any NSURLRequest and some code might be using this specifically to detect applewebdata: requests. Some of the things in Safari that depend on data loads include: the view source window, the load failure error page, and the snippet editor.

Please do the update and either describe testing for #2 or figure out a way to do this w/o making that change.
 

[Maciej Stachowiak]

Comment on attachment 11663 [details]
patch 9

r- for above comments, also, sorry this took so long, hopefully we can get it in soon. Might be simpler to do future reivews if the LayoutTest bits are left out, assuming those don't change, since they form the bulk of the patch.

[Graham Dennis]

Created attachment 12253 [details]
patch 10 (code only)

Patch 10 (code only, the layout test is the same)

Updated to ToT.
The only reason the WebDataProtocol change is necessary is because Safari will crash trying to set an object for a nil key in a dictionary. This seems to be related to the activity viewer. That said, I did some testing to see if anything else broke as a result of this change. I tried opening the 'view source' window on a number of different pages in Safari, and that worked fine (is this just what you wanted checked, or was it something more specific). I tried using the snippet editor, except that it doesn't appear to work in ToT (and still doesn't work with 10725). I also tried entering 'applewebdata://<stuff>' URL's into Safari to see if a load error page would come up, except that Safari just said 'contacting server' in both ToT and with 10725. I also tried 'random://<stuff>' but that also didn't display a load error page (both applewebdata:// and random:// show load error pages in released Safari.).

[Graham Dennis]

I just noticed a commit that fixes the snippet editor. Will update and test.

[Graham Dennis]

(In reply to comment #21)
> I just noticed a commit that fixes the snippet editor. Will update and test.
> 

The 'show' button of the snippet editor works in both ToT and with this patch, the 'Convert' button doesn't work in either situation. Also, 'random://' and 'applewebdata://' URLs both display load error pages both with ToT and with this patch.

[Maciej Stachowiak]

Comment on attachment 12253 [details]
patch 10 (code only)

r=me

[Dan Wood]

I hate to say this after waiting so long to get this landed ... but I just found a bug in the patch, I think.

I have sent a note to Graham privately to see if he can reproduce it and fully analyze it.  

So for now, can you hold off on this?  Mark it r- for now until we verify what is going on?

[Dan Wood]

Well, Graham is unavailable and we can't figure out what the problem is.  I say let's go ahead and land this (since it's been so hard to land) and then later if I can figure out what the problem is, I'll report a new case.

[David Kilzer (:ddkilzer)]

Attachment 12253 [details] (patch 10) has bit-rotted due to loader changes.  I tried to apply it and to clean up the rejects, but too much has changed for me to feel confident about the results.

[Graham Dennis]

Created attachment 12737 [details]
patch 11 (code only)

Patch fixing patch rot. Nothing else fundamental was changed between patch 10 and 11.

I've investigated the problem that Dan was having, and it seems to me to be unrelated to this patch as the problem still occurs when the (previous) patch for 10725 is not applied to WebKit revision 18653. I'm still investigating that issue and will file a new report for it.

[Graham Dennis]

Created attachment 12765 [details]
patch 12 (code only)

Patch fixing patch rot due to removal of WebDataProtocol. Apart from minor tweaking, this patch returns +[NSURL _web_uniqueWebDataURL] because AppKit uses it when turning RTF and RTFD data into a DOMDocument.

[Maciej Stachowiak]

Comment on attachment 12765 [details]
patch 12 (code only)

r=me

[Graham Dennis]

Created attachment 12768 [details]
patch 13 (code only)

Makes TIFF pasting work. (Changes fake URL scheme from -webkit-fake-url to webkit-fake-url.)

[Maciej Stachowiak]

Comment on attachment 12768 [details]
patch 13 (code only)

r=me

[Graham Dennis]

Committed in r19244.

[Mark Rowe (bdash)]

This patch introduced an assertion failure in editing/pasteboard/paste-RTFD.html.  See bug 12474 for more info.