Bug 88736 – http/tests/security/mixedContent/blob-url-in-iframe.html fails on Mac

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

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>

[Adam Barth]

Looks like these are failing because this port doesn't have these features enabled.

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

[Adam Barth]

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

Added filesystem-url-in-iframe.html to the Mac skip list in <http://trac.webkit.org/r119934>.

[Mike West]

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.

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

[Mike West]

Easy, thanks. I'll upload a patch when I get home tonight.

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

[Mike West]

Created attachment 146746 [details]
Patch

[Mike West]

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?

[Adam Barth]

Comment on attachment 146746 [details]
Patch

Thanks Mike!

[WebKit Review Bot]

Comment on attachment 146746 [details]
Patch

Clearing flags on attachment: 146746

Committed r119944: <http://trac.webkit.org/changeset/119944>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

Re-added blob-url-in-iframe.html to the Mac skip list in <http://trac.webkit.org/r120025> since it continued to fail intermittently.

[Mike West]

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

[Kinuko Yasuda]

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

[Mike West]

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.

[Kinuko Yasuda]

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

[Kinuko Yasuda]

I think I found what's happening.  In short, mac's decidePolicyForNavigationAction() is returning PolicyIgnore for blob: URLs therefore the request is silently discarded.

[Kinuko Yasuda]

Created attachment 151696 [details]
Patch

[Ryosuke Niwa]

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

[Alexey Proskuryakov]

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

[Kinuko Yasuda]

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.

[Kinuko Yasuda]

Created attachment 157387 [details]
Patch

[WebKit Review Bot]

Comment on attachment 157387 [details]
Patch

Clearing flags on attachment: 157387

Committed r125235: <http://trac.webkit.org/changeset/125235>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.