Bug 88736

Summary: http/tests/security/mixedContent/blob-url-in-iframe.html fails on Mac
Product: WebKit Reporter: mitz
Component: DOMAssignee: Mike West <mkwst>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, ap, darin, kinuko, mkwst, rniwa, webkit.review.bot
Priority: P2 Keywords: LayoutTestFailure, MakingBotsRed
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description mitz 2012-06-10 09:06:00 PDT
http/tests/security/mixedContent/blob-url-in-iframe.html and http/tests/security/mixedContent/filesystem-url-in-iframe.html have been failing on Lion ever since they were added in <http://trac.webkit.org/r119883>. See for example <http://build.webkit.org/results/Lion%20Release%20(Tests)/r119928%20(9214)/results.html>
Comment 1 Adam Barth 2012-06-10 09:09:22 PDT
Looks like these are failing because this port doesn't have these features enabled.
Comment 2 mitz 2012-06-10 09:11:33 PDT
(In reply to comment #1)
> Looks like these are failing because this port doesn't have these features enabled.

Can http/tests/security/mixedContent/blob-url-in-iframe.html be rewritten to not use the legacy WebKitBlobBuilder class?
Comment 3 Adam Barth 2012-06-10 09:13:42 PDT
> Can http/tests/security/mixedContent/blob-url-in-iframe.html be rewritten to not use the legacy WebKitBlobBuilder class?

I haven't been following the blob builder discussion, but I believe it just needs to create a blob so that it can test how blob URLs are treated.  If there's a better way to create a blob, I suspect the test could use that.
Comment 4 mitz 2012-06-10 09:15:20 PDT
Added filesystem-url-in-iframe.html to the Mac skip list in <http://trac.webkit.org/r119934>.
Comment 5 Mike West 2012-06-10 09:20:42 PDT
Sure, I'm happy to change the test. I'll admit that I haven't been following the API's growth either: I'm guessing you'd prefer the `Blob()` constructor? http://trac.webkit.org/browser/trunk/LayoutTests/fast/files/script-tests/blob-constructor.js looks like a good example I can pull from..

If that's not what you'd like to see, point me in the right direction, and I'm happy to fix the test.
Comment 6 mitz 2012-06-10 09:24:44 PDT
(In reply to comment #5)
> Sure, I'm happy to change the test. I'll admit that I haven't been following the API's growth either: I'm guessing you'd prefer the `Blob()` constructor? http://trac.webkit.org/browser/trunk/LayoutTests/fast/files/script-tests/blob-constructor.js looks like a good example I can pull from..

Yes, that one appears to be the replacement for WebKitBlobBuilder, and is enabled in OS X WebKit.
Comment 7 Mike West 2012-06-10 09:29:57 PDT
Easy, thanks. I'll upload a patch when I get home tonight.
Comment 8 mitz 2012-06-10 09:33:59 PDT
(In reply to comment #7)
> Easy, thanks. I'll upload a patch when I get home tonight.

Thank you. I’ve temporarily added the test to platform/mac/Skipped in <http://trac.webkit.org/r119935>. Please include removing it from the skip list in your patch that makes it not depend on WebKitBlobBuilder.
Comment 9 Mike West 2012-06-10 11:53:46 PDT
Created attachment 146746 [details]
Patch
Comment 10 Mike West 2012-06-10 11:55:31 PDT
Comment on attachment 146746 [details]
Patch

The attached patch should run correctly in the absence of WebKitBlobBuilder, and reenables the test for the mac platform. WDYT?
Comment 11 Adam Barth 2012-06-10 12:14:34 PDT
Comment on attachment 146746 [details]
Patch

Thanks Mike!
Comment 12 WebKit Review Bot 2012-06-10 12:49:11 PDT
Comment on attachment 146746 [details]
Patch

Clearing flags on attachment: 146746

Committed r119944: <http://trac.webkit.org/changeset/119944>
Comment 13 WebKit Review Bot 2012-06-10 12:49:16 PDT
All reviewed patches have been landed.  Closing bug.
Comment 14 mitz 2012-06-11 18:13:04 PDT
Re-added blob-url-in-iframe.html to the Mac skip list in <http://trac.webkit.org/r120025> since it continued to fail intermittently.
Comment 15 Mike West 2012-06-12 00:46:43 PDT
Hi Kinuko! Would you mind taking a look at this?

http/tests/security/mixedContent/blob-url-in-iframe.html is failing, but only on Lion WebKit. I don't see anything in the code that would have this sort of platform-specific behavior, but I'm hoping you might have some insight. :)
Comment 16 Kinuko Yasuda 2012-06-12 08:23:14 PDT
(In reply to comment #15)
> Hi Kinuko! Would you mind taking a look at this?
> 
> http/tests/security/mixedContent/blob-url-in-iframe.html is failing, but only on Lion WebKit. I don't see anything in the code that would have this sort of platform-specific behavior, but I'm hoping you might have some insight. :)

Hmm I can't think of any platform-specific issue here, but the test seems to fail on my local Lion WebKit.
Comment 17 Mike West 2012-06-12 08:27:09 PDT
On my machine, it's simply not loading the blob URL into the iframe. The onload event isn't generated, but neither is an onerror event. It just doesn't do anything at all.

I'll investigate a bit more tonight, but I'm hoping you can point me at some bits of the code that might be responsible. It seems like a real bug, not a bad test.
Comment 18 Kinuko Yasuda 2012-06-13 00:15:09 PDT
(In reply to comment #17)
> On my machine, it's simply not loading the blob URL into the iframe. The onload event isn't generated, but neither is an onerror event. It just doesn't do anything at all.
> 
> I'll investigate a bit more tonight, but I'm hoping you can point me at some bits of the code that might be responsible. It seems like a real bug, not a bad test.

Yeah it looks like a real bug. I also observed that it's not loading the blob URL into the iframe.  The method that is modified when the test is added, isMixedContent, doesn't seem to be called for the blob either.

For non-chromium port Blob code goes through BlobResourceHandle and BlobRegistryImpl in platform/network.  The part may have some clue?
Comment 19 Kinuko Yasuda 2012-07-11 06:45:43 PDT
I think I found what's happening.  In short, mac's decidePolicyForNavigationAction() is returning PolicyIgnore for blob: URLs therefore the request is silently discarded.
Comment 20 Kinuko Yasuda 2012-07-11 06:46:59 PDT
Created attachment 151696 [details]
Patch
Comment 21 Ryosuke Niwa 2012-08-07 22:55:18 PDT
Comment on attachment 151696 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=151696&action=review

This patch looks sane but maybe ap or mitz should review it?

> Source/WebKit/mac/ChangeLog:8
> +        WebView::_canHandleRequest should return true for "blob: URL so that it can be shown in iframe.

Nit: dangling ".
Comment 22 Alexey Proskuryakov 2012-08-08 09:25:43 PDT
Comment on attachment 151696 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=151696&action=review

> Source/WebKit/mac/WebView/WebView.mm:1765
> +    return [scheme _webkit_isCaseInsensitiveEqualToString:@"applewebdata"] || [scheme _webkit_isCaseInsensitiveEqualToString:@"blob"];

Shouldn't the new case should be inside #if ENABLE(BLOB)?
Comment 23 Kinuko Yasuda 2012-08-08 22:21:32 PDT
Comment on attachment 151696 [details]
Patch

Thanks for reviewing,

View in context: https://bugs.webkit.org/attachment.cgi?id=151696&action=review

>> Source/WebKit/mac/ChangeLog:8
>> +        WebView::_canHandleRequest should return true for "blob: URL so that it can be shown in iframe.
> 
> Nit: dangling ".

Fixed.

>> Source/WebKit/mac/WebView/WebView.mm:1765
>> +    return [scheme _webkit_isCaseInsensitiveEqualToString:@"applewebdata"] || [scheme _webkit_isCaseInsensitiveEqualToString:@"blob"];
> 
> Shouldn't the new case should be inside #if ENABLE(BLOB)?

Fixed.
Comment 24 Kinuko Yasuda 2012-08-08 22:22:20 PDT
Created attachment 157387 [details]
Patch
Comment 25 WebKit Review Bot 2012-08-09 19:12:14 PDT
Comment on attachment 157387 [details]
Patch

Clearing flags on attachment: 157387

Committed r125235: <http://trac.webkit.org/changeset/125235>
Comment 26 WebKit Review Bot 2012-08-09 19:12:19 PDT
All reviewed patches have been landed.  Closing bug.