Summary: | http/tests/security/mixedContent/blob-url-in-iframe.html fails on Mac | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | mitz | ||||||||
Component: | DOM | Assignee: | 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
mitz
2012-06-10 09:06:00 PDT
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? > 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>. 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. 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. Created attachment 146746 [details]
Patch
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 on attachment 146746 [details]
Patch
Thanks Mike!
Comment on attachment 146746 [details] Patch Clearing flags on attachment: 146746 Committed r119944: <http://trac.webkit.org/changeset/119944> 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. 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. :) (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. 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. (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? I think I found what's happening. In short, mac's decidePolicyForNavigationAction() is returning PolicyIgnore for blob: URLs therefore the request is silently discarded. Created attachment 151696 [details]
Patch
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 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 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. Created attachment 157387 [details]
Patch
Comment on attachment 157387 [details] Patch Clearing flags on attachment: 157387 Committed r125235: <http://trac.webkit.org/changeset/125235> All reviewed patches have been landed. Closing bug. |