The Android web view application could provide the default poster for a video that doesn't have the poster attribute. To provide the default poster, the application must set defaultVideoPosterURL setting and return the image in the response of that URL.
Created attachment 189171 [details] Patch
Created attachment 189174 [details] Patch
Comment on attachment 189174 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=189174&action=review > Source/WebCore/ChangeLog:15 > + No new tests, covered by existing tests. How is this new functionallity covered by existing tests?
Sorry, I just found out I could write a Layout test to test this feature, I am working on it.
Created attachment 189357 [details] Patch
Created attachment 189368 [details] Patch
GTK build fails because the new symbols are not referenced in Source/autotools/symbols.filter
Comment on attachment 189368 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=189368&action=review Marking r- because of the GTK problem. > LayoutTests/media/video-default-poster.html:17 > + document.getElementById("result-no-poster").innerText = > + poster.value == default_poster ? "PASS" : "FAIL"; Nit: I think the line break + ternary make this more difficult to understand instead of easier. > LayoutTests/media/video-default-poster.html:20 > + } else { > + document.getElementById("result-no-poster").innerText = "FAIL: poster attribute is null"; > + } Nit: single line if conditionals don't need braces. > LayoutTests/media/video-default-poster.html:27 > + if (poster) { > + document.getElementById("result-has-poster").innerText = > + poster.value == "content/abe.png" ? "PASS" : "FAIL: poster was changed"; > + } else { > + document.getElementById("result-has-poster").innerText = "FAIL: poster attribute is null"; > + } Dittos. > LayoutTests/media/video-no-default-poster.html:21 > + document.getElementById("result-no-poster").innerText = > + poster ? "FAIL : poster was added." : "PASS"; > + poster = document.getElementById("video-has-poster").getAttributeNode("poster"); > + if (poster) { > + document.getElementById("result-has-poster").innerText = > + poster.value == "content/abe.png" ? "PASS" : "FAIL: poster was changed"; > + } else { > + document.getElementById("result-has-poster").innerText = "FAIL: poster attribute is null"; > + } Dittos.
Created attachment 189562 [details] Patch
Comment on attachment 189562 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=189562&action=review > Source/WebCore/ChangeLog:22 > + (WebCore::HTMLVideoElement::attach): > + * page/Settings.in: > + * testing/InternalSettings.cpp: > + (WebCore::InternalSettings::Backup::Backup): > + (WebCore::InternalSettings::Backup::restoreTo): > + (WebCore::InternalSettings::setDefaultVideoPosterURL): > + (WebCore): > + * testing/InternalSettings.h: > + (Backup): > + (InternalSettings): > + * testing/InternalSettings.idl: Sorry I didn't notice this before, but it is helpful to have per-method comments about what changed here. > LayoutTests/media/video-default-poster.html:11 > + var default_poster = "about:blank"; Why use an invalid poster? > LayoutTests/media/video-default-poster.html:17 > + if (poster) { > + if (poster.value) > + document.getElementById("result-no-poster").innerText = "PASS"; Shouldn't this test the poster value before declaring success? > ChangeLog:8 > + * Source/autotools/symbols.filter: Ditto.
Created attachment 189596 [details] Patch
Comment on attachment 189562 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=189562&action=review >> Source/WebCore/ChangeLog:22 >> + * testing/InternalSettings.idl: > > Sorry I didn't notice this before, but it is helpful to have per-method comments about what changed here. Never mind, Done >> LayoutTests/media/video-default-poster.html:11 >> + var default_poster = "about:blank"; > > Why use an invalid poster? I were trying to use content/abe.png, but the test failed because content/abe.png is not a valid URL for KURL's GURL implementation. The default poster URL couldn't be related path in practice, so it shouldn't be an issue. >> LayoutTests/media/video-default-poster.html:17 >> + document.getElementById("result-no-poster").innerText = "PASS"; > > Shouldn't this test the poster value before declaring success? Done >> ChangeLog:8 >> + * Source/autotools/symbols.filter: > > Ditto. Done
Comment on attachment 189596 [details] Patch Attachment 189596 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/16722088
(In reply to comment #13) > (From update of attachment 189596 [details]) > Attachment 189596 [details] did not pass win-ews (win): > Output: http://queues.webkit.org/results/16722088 10>WebCoreTestSupport.lib(InternalSettings.obj) : error LNK2019: unresolved external symbol "public: __thiscall WebCore::KURL::KURL(enum WebCore::ParsedURLStringTag,class WTF::String const &)" (??0KURL@WebCore@@QAE@W4ParsedURLStringTag@1@ABVString@WTF@@@Z) referenced in function "public: void __thiscall WebCore::InternalSettings::Backup::restoreTo(class WebCore::Settings *)" (?restoreTo@Backup@InternalSettings@WebCore@@QAEXPAVSettings@3@@Z)
Created attachment 189829 [details] Patch
This should fix win bot.
(In reply to comment #16) > This should fix win bot. Looks like you need to rebase the patch: Last 500 characters of output: ebKitExports.def.in.rej patching file Source/autotools/symbols.filter Hunk #1 succeeded at 272 (offset 10 lines). patching file LayoutTests/ChangeLog Hunk #1 succeeded at 1 with fuzz 3. patching file LayoutTests/media/video-default-poster-expected.txt patching file LayoutTests/media/video-default-poster.html patching file LayoutTests/media/video-no-default-poster-expected.txt patching file LayoutTests/media/video-no-default-poster.html patching file ChangeLog Hunk #1 succeeded at 1 with fuzz 3.
Created attachment 189839 [details] Patch
Why win bot didn't showup?
Created attachment 190080 [details] Patch
Rebase again.
(In reply to comment #12) > >> LayoutTests/media/video-default-poster.html:11 > >> + var default_poster = "about:blank"; > > > > Why use an invalid poster? > > I were trying to use content/abe.png, but the test failed because content/abe.png is not a valid URL for KURL's GURL implementation. > The default poster URL couldn't be related path in practice, so it shouldn't be an issue. The test can construct an absolute URL for content/abe.png.
(In reply to comment #22) > (In reply to comment #12) > > >> LayoutTests/media/video-default-poster.html:11 > > >> + var default_poster = "about:blank"; > > > > > > Why use an invalid poster? > > > > I were trying to use content/abe.png, but the test failed because content/abe.png is not a valid URL for KURL's GURL implementation. > > The default poster URL couldn't be related path in practice, so it shouldn't be an issue. > > The test can construct an absolute URL for content/abe.png. Would you like point an example to me, thanks?
(In reply to comment #23) > (In reply to comment #22) > > (In reply to comment #12) > > > >> LayoutTests/media/video-default-poster.html:11 > > > >> + var default_poster = "about:blank"; > > > > > > > > Why use an invalid poster? > > > > > > I were trying to use content/abe.png, but the test failed because content/abe.png is not a valid URL for KURL's GURL implementation. > > > The default poster URL couldn't be related path in practice, so it shouldn't be an issue. > > > > The test can construct an absolute URL for content/abe.png. > > Would you like point an example to me, thanks? I just found out the testcase's URL with window.location, it is the file name itself, like 'video-default-poster.html', so I guess use 'about:blank' is a good way here.
(In reply to comment #24) > I just found out the testcase's URL with window.location, it is the file name itself, like 'video-default-poster.html', so I guess use 'about:blank' is a good way here. Did you try document.URL?
(In reply to comment #25) > (In reply to comment #24) > > I just found out the testcase's URL with window.location, it is the file name itself, like 'video-default-poster.html', so I guess use 'about:blank' is a good way here. > > Did you try document.URL? It is same, gives file name itself.
(In reply to comment #26) > It is same, gives file name itself. Strange, given the many tests that use document.URL such as fast/loader/external-script-URL-location.html.
(In reply to comment #26) > It is same, gives file name itself. Something is strange. I modified your test to just log window.location.href: <!DOCTYPE html> <html> <head> <script> if (window.testRunner) { testRunner.dumpAsText(); testRunner.waitUntilDone(); } if (window.internals) { addEventListener("load", function() { alert(window.location.href); testRunner.notifyDone(); }, false); } </script> </head> <body> <pre id="result-no-poster"></pre> <pre id="result-has-poster"></pre> <video id="video-no-poster" src="content/test.mp4" preload="none" /> <video id="video-has-poster" src="content/test.mp4" poster="content/abe.png" preload="none" /> </body> </html> and the result is ALERT: file:///Volumes/User/WebKit/Sources/OpenSource/LayoutTests/media/temp.html
Would you like me to know how did you run it? I ran the test with the following command, perl Tools/Scripts/run-webkit-tests --chromium --debug LayoutTests/media/video-default-poster.html I think this might be the reason I only got the file name itself.
(In reply to comment #27) > (In reply to comment #26) > > It is same, gives file name itself. > > Strange, given the many tests that use document.URL such as fast/loader/external-script-URL-location.html. This test also passed when I ran it with perl Tools/Scripts/run-webkit-tests --chromium --debug LayoutTests/fast/loader/external-script-URL-location.html The test passed even the Document.URL doesn't have '/'.
It seemed that console.log only output the relative path. I will provide another patch.
Created attachment 190338 [details] Patch
Changed to construct URL in Javascript, PTAL
Comment on attachment 190338 [details] Patch Thank you for updating the test!
Comment on attachment 190338 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=190338&action=review > Source/WebCore/html/HTMLVideoElement.cpp:90 > + setAttribute(posterAttr, defaultVideoPosterURL.string()); I don’t know why I didn’t notice this earlier. This is not the right way to do it! We should do this logic when getting the poster attribute. We shouldn’t actually set the poster attribute on an element. It’s not good to have an element that modifies itself during the attach process -- it could even lead to a security bug.
(In reply to comment #35) > (From update of attachment 190338 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=190338&action=review > > > Source/WebCore/html/HTMLVideoElement.cpp:90 > > + setAttribute(posterAttr, defaultVideoPosterURL.string()); > > I don’t know why I didn’t notice this earlier. This is not the right way to do it! We should do this logic when getting the poster attribute. We shouldn’t actually set the poster attribute on an element. It’s not good to have an element that modifies itself during the attach process -- it could even lead to a security bug. Would you like me know why there is a security bug here? What the right way to do this?
Hi Darin, Only Android web view application will enable this feature, beside this there is no API or UI coud access this, the application could do whatever they would like and it is the user's choice to trust the application. It shouldn't have security issue here. In another word, it has no impact to the rest of the world. Thanks
Created attachment 190796 [details] Patch
ping...
(In reply to comment #37) > Only Android web view application will enable this feature, beside this there is no API or UI coud access this, the application could do whatever they would like and it is the user's choice to trust the application. It shouldn't have security issue here. Your analysis is incorrect. If this Android web view app loads content from the web, the web content could exploit the bug and take advantage of it to run arbitrary code.
(In reply to comment #40) > (In reply to comment #37) > > Only Android web view application will enable this feature, beside this there is no API or UI coud access this, the application could do whatever they would like and it is the user's choice to trust the application. It shouldn't have security issue here. > > Your analysis is incorrect. If this Android web view app loads content from the web, the web content could exploit the bug and take advantage of it to run arbitrary code. Thanks for reply, I still not quite understand how the security issue could happen? Did you mean load the poster image could run arbitrary code?
The correct way to do this is to put this logic into the code that fetches the poster. We should not actually change the poster attribute on the DOM element, instead we should fetch the appropriate poster. The way to do this would be: A) Replace the Element::imageSourceAttributeName function with an Element::imageSourceURL function that returns the imageSourceURL as a const AtomicString&. The body will be the same as before, it will just also include a call to getAttribute. Also will need to revise the four classes that override that function. B) Add a new HTMLVideoElement::posterImageURL function that implements the default poster URL logic. C) Update the four functions that get the poster attribute to handle poster loading and display to call posterImageURL. 1) HTMLVideoElement::imageSourceURL. 2) HTMLVideoElement::setDisplayMode. 3) HTMLVideoElement::updateDisplayState. 4) HTMLMediaElement::getPluginProxyParams. Will need to cast to HTMLVideoElement after checking isVideo. That’s the correct way to fix this.
(In reply to comment #41) > Did you mean load the poster image could run arbitrary code? Setting a poster DOM attribute will allow handlers on the webpage to run arbitrary JavaScript during the attach process. Running arbitrary JavaScript at that time can result in the engine following stale pointers, which is exploitable to run arbitrary code.
Created attachment 191627 [details] Patch
Comment on attachment 191627 [details] Patch Not for review, just want to try
Comment on attachment 191627 [details] Patch Drive by comment: I would recommend only removing extraneous white-space in functions where you are also changing code. Removing it as you have here makes it harder to pick out what you have actually changed.
Created attachment 191830 [details] Patch
I should add new symbols in Source/WebCore/WebCore.order, but don't have corresponding platform, will add them once error appears in bots.
Comment on attachment 191830 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=191830&action=review > Source/WebCore/ChangeLog:16 > + Nowhere here do you explain why imageSourceAttributeName must be changed to imageSourceURL, which seems to be a signficant outcome of this patch.
(In reply to comment #49) > (From update of attachment 191830 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=191830&action=review > > > Source/WebCore/ChangeLog:16 > > + > > Nowhere here do you explain why imageSourceAttributeName must be changed to imageSourceURL, which seems to be a signficant outcome of this patch. I followed Darin Adler's suggestion (refer comments 42), actually, the return value from imageSourceAttributeName() ends up with getAttribute(imageSourceAttributeName()), it's exactly same as the default implementation of imageSourceURL() does.
(In reply to comment #50) > (In reply to comment #49) > > (From update of attachment 191830 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=191830&action=review > > > > > Source/WebCore/ChangeLog:16 > > > + > > > > Nowhere here do you explain why imageSourceAttributeName must be changed to imageSourceURL, which seems to be a signficant outcome of this patch. > > I followed Darin Adler's suggestion (refer comments 42), actually, the return value from imageSourceAttributeName() ends up with getAttribute(imageSourceAttributeName()), it's exactly same as the default implementation of imageSourceURL() does. Yes, but I believe the ChangeLog should say something. Probably just repeat Darin's outline of the change.
Created attachment 191856 [details] Patch
Update ChangeLog, thanks
Created attachment 192167 [details] Patch
Comment on attachment 192167 [details] Patch Looks good to me, but I think Darin should give the final word. I'm ultimately not an expert in this area.
Comment on attachment 192167 [details] Patch Attachment 192167 [details] did not pass mac-ews (mac): Output: http://webkit-commit-queue.appspot.com/results/17007165 New failing tests: editing/selection/selection-modify-crash.html
Comment on attachment 192167 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=192167&action=review Looks OK, but breaks whitespace handling in video element poster URLs. I’m surprised there was no test for this that is broken. Please fix that. > Source/WebCore/html/HTMLVideoElement.cpp:312 > + const AtomicString& url = imageSourceURL(); It’s a shame to make a virtual function call here. No big deal, but not ideal either. > Source/WebCore/html/HTMLVideoElement.cpp:315 > + if (url.isEmpty()) > + return KURL(); > + return document()->completeURL(url); This is missing a call to stripLeadingAndTrailingHTMLSpaces.
Created attachment 192318 [details] Patch
Comment on attachment 192167 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=192167&action=review >> Source/WebCore/html/HTMLVideoElement.cpp:315 >> + return document()->completeURL(url); > > This is missing a call to stripLeadingAndTrailingHTMLSpaces. Done
Comment on attachment 192318 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=192318&action=review > Source/WebCore/html/HTMLVideoElement.cpp:189 > + const AtomicString& url = getAttribute(posterAttr); > + if (!url.isEmpty()) > + return url; On reflection, it’s a little strange here to special case only an empty URL without considering whitespace stripping. I could imagine special casing only a null URL, which means we’d only do this when the poster attribute is entirely missing. Or special casing any URL that ends up empty, and therefore is something we won't even try to load. > Source/WebCore/html/HTMLVideoElement.cpp:316 > + const AtomicString& url = imageSourceURL(); > + if (url.isEmpty()) > + return KURL(); > + return document()->completeURL(stripLeadingAndTrailingHTMLSpaces(url)); This is still wrong. The stripping of needs to be done before the isEmpty check. Just look at the code in getNonEmptyURLAttribute and match its behavior exactly. That’s the problem with copied and pasted code, I guess. Hard to get it right! > Source/WebCore/html/HTMLVideoElement.h:94 > + AtomicString m_defaultPosterURL; It’s a little strange to store a copy of this in every video element. There’s no reason we couldn’t fetch it from the document settings when we need it instead of when we create the video element.
Created attachment 192766 [details] Patch
Comment on attachment 192318 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=192318&action=review >> Source/WebCore/html/HTMLVideoElement.cpp:189 >> + return url; > > On reflection, it’s a little strange here to special case only an empty URL without considering whitespace stripping. I could imagine special casing only a null URL, which means we’d only do this when the poster attribute is entirely missing. Or special casing any URL that ends up empty, and therefore is something we won't even try to load. Thanks, I striped space for isEmpty() checking, but returned the original value, I think imageSoureURL is supposed to return unstriped value. >> Source/WebCore/html/HTMLVideoElement.cpp:316 >> + return document()->completeURL(stripLeadingAndTrailingHTMLSpaces(url)); > > This is still wrong. The stripping of needs to be done before the isEmpty check. Just look at the code in getNonEmptyURLAttribute and match its behavior exactly. That’s the problem with copied and pasted code, I guess. Hard to get it right! Done >> Source/WebCore/html/HTMLVideoElement.h:94 >> + AtomicString m_defaultPosterURL; > > It’s a little strange to store a copy of this in every video element. There’s no reason we couldn’t fetch it from the document settings when we need it instead of when we create the video element. The document->settings()->defaultVideoPosterURL() returns value, but imageSourceURL() returns a reference, I didn't find other workaround, so have to hold the value here. :( I just guess the settings doesn't return reference might because the settings could be changed during the page's life cycle.
PTAL when you have time
Comment on attachment 192766 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=192766&action=review review- because the whitespace stripping is still wrong in this patch; I’m guessing you uploaded the wrong one > Source/WebCore/ChangeLog:37 > + * WebCore.order: Replace imageSourceAttributeName() with imageSourceURL() This file does not need to be updated. You can just remove this change from your patch. > Source/WebCore/ChangeLog:39 > + * dom/Element.cpp: Replace imageSourceAttributeName() with imageSourceURL() > + (WebCore::Element::imageSourceURL): Comments should go on the functions, not the entire file, unless there is no function name. > Source/WebCore/ChangeLog:41 > + (Element): Bogus lines like this should just be deleted from the ChangeLog. The script generates them. > Source/WebCore/myChangeLog:1 > +2013-03-06 Tao Bai <michaelbai@chromium.org> Please don’t land this second change log file! > Source/WebCore/html/HTMLVideoElement.cpp:189 > + const AtomicString& url = getAttribute(posterAttr); > + if (!url.isEmpty()) > + return url; As I mentioned in my last patch, it’s not right to check isEmpty here. It would be OK to check isNull, or to check isEmpty after calling stripLeadingAndTrailingHTMLSpaces. > Source/WebCore/html/HTMLVideoElement.cpp:316 > + const AtomicString& url = imageSourceURL(); > + if (url.isEmpty()) > + return KURL(); > + return document()->completeURL(stripLeadingAndTrailingHTMLSpaces(url)); The stripLeadingAndTrailingHTMLSpaces call needs to be *before* calling isEmpty. Isn’t that why I said review- last time? Did you upload the wrong version of the patch? > Source/WebCore/html/HTMLVideoElement.h:68 > + KURL posterImageURL(); Should be const.
Created attachment 192945 [details] Patch
Comment on attachment 192766 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=192766&action=review >> Source/WebCore/ChangeLog:37 >> + * WebCore.order: Replace imageSourceAttributeName() with imageSourceURL() > > This file does not need to be updated. You can just remove this change from your patch. Done >> Source/WebCore/ChangeLog:39 >> + (WebCore::Element::imageSourceURL): > > Comments should go on the functions, not the entire file, unless there is no function name. Done >> Source/WebCore/ChangeLog:41 >> + (Element): > > Bogus lines like this should just be deleted from the ChangeLog. The script generates them. Done >> Source/WebCore/myChangeLog:1 >> +2013-03-06 Tao Bai <michaelbai@chromium.org> > > Please don’t land this second change log file! Sorry, it is my bad, this is just wrong patch >> Source/WebCore/html/HTMLVideoElement.cpp:189 >> + return url; > > As I mentioned in my last patch, it’s not right to check isEmpty here. It would be OK to check isNull, or to check isEmpty after calling stripLeadingAndTrailingHTMLSpaces. Sorry for the wrong patch. >> Source/WebCore/html/HTMLVideoElement.cpp:316 >> + return document()->completeURL(stripLeadingAndTrailingHTMLSpaces(url)); > > The stripLeadingAndTrailingHTMLSpaces call needs to be *before* calling isEmpty. Isn’t that why I said review- last time? Did you upload the wrong version of the patch? Sorry again. >> Source/WebCore/html/HTMLVideoElement.h:68 >> + KURL posterImageURL(); > > Should be const. Done
Comment on attachment 192945 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=192945&action=review > Source/WebCore/html/HTMLVideoElement.cpp:188 > + if (!stripLeadingAndTrailingHTMLSpaces(url).isEmpty()) Some day we may want to replace this by an isAllHTMLSpaces function that does not require copying the string even if the string contains leading or trailing spaces. > Source/WebCore/html/HTMLVideoElement.h:72 > + const KURL posterImageURL(); The const here before KURL is unneeded and nearly meaningless. It just means that you can’t call non-const member functions on the return value from this function. What I meant when I said that this function should be const is that there should be a const *after* the parentheses, allowing this to be called on a const HTMLVideoElement.
Created attachment 192979 [details] Patch
Comment on attachment 192945 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=192945&action=review >> Source/WebCore/html/HTMLVideoElement.h:72 >> + const KURL posterImageURL(); > > The const here before KURL is unneeded and nearly meaningless. It just means that you can’t call non-const member functions on the return value from this function. > > What I meant when I said that this function should be const is that there should be a const *after* the parentheses, allowing this to be called on a const HTMLVideoElement. Done
Comment on attachment 192979 [details] Patch Clearing flags on attachment: 192979 Committed r145750: <http://trac.webkit.org/changeset/145750>
All reviewed patches have been landed. Closing bug.
Comment on attachment 192979 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=192979&action=review > Source/WebCore/html/HTMLVideoElement.cpp:320 > + const AtomicString& url = stripLeadingAndTrailingHTMLSpaces(imageSourceURL()); After this landed, I noticed a mistake here. This should be typed either const String& or String. Using the type "const AtomicString&" means we’ll do an extra hash lookup to make an atomic string, but there is no value to doing that. We need a followup.
Will do