Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2371573002:
Fix download attr on anchor allows download without user interaction
DescriptionFix Download attr on anchor allows download without user interaction
HTMLAnchorElement which has both href and download should not triggered
if there is no user interaction, and have to throw an
"InvalidAccessError" DOMException.
Now Download and href is not triggered, but not thorwing DOMException
Added TODO in Source Code
Spec-Url: https://html.spec.whatwg.org/#the-a-element:the-a-element-9
BUG=649918
Patch Set 1 #Patch Set 2 : Removed unnecessary header #
Total comments: 2
Messages
Total messages: 26 (12 generated)
Description was changed from ========== fix Download attr on anchor allows download without user interaction HTMLAnchorElement which has both href and download should not triggered if there is no user interaction, and have to throw an "InvalidAccessError" DOMException. Now Download and href is not triggered, but not thorwing DOMException Added TODO in Source Code Spec-Url: https://html.spec.whatwg.org/#the-a-element:the-a-element-9 BUG=649918 patch from issue 2369453006 at patchset 1 (http://crrev.com/2369453006#ps1) ========== to ========== Fix Download attr on anchor allows download without user interaction HTMLAnchorElement which has both href and download should not triggered if there is no user interaction, and have to throw an "InvalidAccessError" DOMException. Now Download and href is not triggered, but not thorwing DOMException Added TODO in Source Code Spec-Url: https://html.spec.whatwg.org/#the-a-element:the-a-element-9 BUG=649918 patch from issue 2369453006 at patchset 1 (http://crrev.com/2369453006#ps1) ==========
jhwon0415@gmail.com changed reviewers: + jinho.bang@samsung.com
Fixed bug, but I failed to raise exception
jinho.bang@samsung.com changed reviewers: + dtapuska@chromium.org, rbyers@chromium.org
Description was changed from ========== Fix Download attr on anchor allows download without user interaction HTMLAnchorElement which has both href and download should not triggered if there is no user interaction, and have to throw an "InvalidAccessError" DOMException. Now Download and href is not triggered, but not thorwing DOMException Added TODO in Source Code Spec-Url: https://html.spec.whatwg.org/#the-a-element:the-a-element-9 BUG=649918 patch from issue 2369453006 at patchset 1 (http://crrev.com/2369453006#ps1) ========== to ========== Fix Download attr on anchor allows download without user interaction HTMLAnchorElement which has both href and download should not triggered if there is no user interaction, and have to throw an "InvalidAccessError" DOMException. Now Download and href is not triggered, but not thorwing DOMException Added TODO in Source Code Spec-Url: https://html.spec.whatwg.org/#the-a-element:the-a-element-9 BUG=649918 ==========
+ dtapuska@ and rbyers@ for reviewers.
For reviewer, Chromium/Blink Hackathon is going on now in our country. jhwon0415@ is a university student and I'm a his mentor. I assigned him because this issue looks simple and easy. Could you please review this for him?
jhwon0415@, I think we need a unit tests. Could you add a unit tests for this change? Please see: - https://www.chromium.org/blink/unittesting Example: - https://codereview.chromium.org/1668553003/patch/100001/110006 You can refer to all *Test.cpp in cs.chromium.org.
On 2016/09/27 13:42:21, zino wrote: > For reviewer, > > Chromium/Blink Hackathon is going on now in our country. > jhwon0415@ is a university student and I'm a his mentor. > I assigned him because this issue looks simple and easy. > Could you please review this for him? Thanks for the context, and thanks for contributing! This seems like a reasonable step forward to me, but yeah a test is probably necessary to ensure it doesn't regress. https://codereview.chromium.org/2371573002/diff/20001/AUTHORS File AUTHORS (right): https://codereview.chromium.org/2371573002/diff/20001/AUTHORS#newcode259 AUTHORS:259: Huiwon Jo <jhwon0415@gmail.com> For the record: I verified you've signed the chromium CLA. Thank you! https://codereview.chromium.org/2371573002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLAnchorElement.cpp (right): https://codereview.chromium.org/2371573002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLAnchorElement.cpp:436: // TODO: Throw an "InvalidAccessError" DOMException. I'm OK leaving this as follow-up work (it's probably non-trivial, have to plumb an ExceptionState through), but the comment should reference an outstanding bug. Feel free to just refer to the existing bug and we can leave it open to track fixing this completely.
The CQ bit was checked by jinho.bang@samsung.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
jhwon0415@gmail.com changed reviewers: + asanka@chromium.org, rdsmith@chromium.org, svaldez@chromium.org
I fixed bug by calling if(isTrusted()) before actual download. Because this method block javascript-generated click() function which trigger anchorelement's download, lots of browsertest got falure. To fix this issue, can i use runtime flag? Like if(isTruested() || isRuntimeFlag()) and setting runtime flag before browertest.
jhwon0415@gmail.com changed reviewers: - rdsmith@chromium.org
jhwon0415@gmail.com changed reviewers: - dtapuska@chromium.org
On 2016/09/29 16:05:43, jhwon0415 wrote: > I fixed bug by calling if(isTrusted()) before actual download. > Because this method block javascript-generated click() function which trigger > anchorelement's download, lots of browsertest got falure. > To fix this issue, can i use runtime flag? > Like if(isTruested() || isRuntimeFlag()) > and setting runtime flag before browertest. +1
On 2016/09/29 16:30:20, zino wrote: > On 2016/09/29 16:05:43, jhwon0415 wrote: > > I fixed bug by calling if(isTrusted()) before actual download. > > Because this method block javascript-generated click() function which trigger > > anchorelement's download, lots of browsertest got falure. > > To fix this issue, can i use runtime flag? > > Like if(isTruested() || isRuntimeFlag()) > > and setting runtime flag before browertest. > > +1 IMHO, the runtime flag seems to be the only way to fix regressions of existing tests.
On 2016/09/29 16:35:21, zino wrote: > On 2016/09/29 16:30:20, zino wrote: > > On 2016/09/29 16:05:43, jhwon0415 wrote: > > > I fixed bug by calling if(isTrusted()) before actual download. > > > Because this method block javascript-generated click() function which > trigger > > > anchorelement's download, lots of browsertest got falure. > > > To fix this issue, can i use runtime flag? > > > Like if(isTruested() || isRuntimeFlag()) > > > and setting runtime flag before browertest. > > > > +1 > > IMHO, the runtime flag seems to be the only way to fix regressions of existing > tests. I don't think adding a runtime flag is very good. The tests should be updated so they inject input not via javascript. But actually generate a sequence of events via something like the eventSender, or other mechanism. You probably can generate a click event in the browser tests as opposed to calling click here: https://cs.chromium.org/chromium/src/chrome/test/data/downloads/download-dang...
On 2016/09/29 19:51:44, dtapuska wrote: > On 2016/09/29 16:35:21, zino wrote: > > On 2016/09/29 16:30:20, zino wrote: > > > On 2016/09/29 16:05:43, jhwon0415 wrote: > > > > I fixed bug by calling if(isTrusted()) before actual download. > > > > Because this method block javascript-generated click() function which > > trigger > > > > anchorelement's download, lots of browsertest got falure. > > > > To fix this issue, can i use runtime flag? > > > > Like if(isTruested() || isRuntimeFlag()) > > > > and setting runtime flag before browertest. > > > > > > +1 > > > > IMHO, the runtime flag seems to be the only way to fix regressions of existing > > tests. > > I don't think adding a runtime flag is very good. The tests should be updated so > they inject input not via javascript. But actually generate a sequence of events > via something like the eventSender, or other mechanism. > > You probably can generate a click event in the browser tests as opposed to > calling click here: > https://cs.chromium.org/chromium/src/chrome/test/data/downloads/download-dang... It looks like there's only actually about 7 impacted tests, right? I agree we should just update the tests to generate trusted input instead of click(). dtapuska@ recently did a lot of this for http://crbug.com/520519. If more than a handful of tests ended up being broken, then that would be an indication that this might be a bigger breaking change for the web than we thought...
I searched code for few hours,, and I couldn't find way to make trusted input in browser test with gmock. Most of tests dtapuska chaged were layout test. Is there any sample code to generate trusted input is gmock? On 2016/09/29 20:17:48, Rick Byers wrote: > On 2016/09/29 19:51:44, dtapuska wrote: > > On 2016/09/29 16:35:21, zino wrote: > > > On 2016/09/29 16:30:20, zino wrote: > > > > On 2016/09/29 16:05:43, jhwon0415 wrote: > > > > > I fixed bug by calling if(isTrusted()) before actual download. > > > > > Because this method block javascript-generated click() function which > > > trigger > > > > > anchorelement's download, lots of browsertest got falure. > > > > > To fix this issue, can i use runtime flag? > > > > > Like if(isTruested() || isRuntimeFlag()) > > > > > and setting runtime flag before browertest. > > > > > > > > +1 > > > > > > IMHO, the runtime flag seems to be the only way to fix regressions of > existing > > > tests. > > > > I don't think adding a runtime flag is very good. The tests should be updated > so > > they inject input not via javascript. But actually generate a sequence of > events > > via something like the eventSender, or other mechanism. > > > > You probably can generate a click event in the browser tests as opposed to > > calling click here: > > > https://cs.chromium.org/chromium/src/chrome/test/data/downloads/download-dang... > > It looks like there's only actually about 7 impacted tests, right? I agree we > should just update the tests to generate trusted input instead of click(). > dtapuska@ recently did a lot of this for http://crbug.com/520519. If more than > a handful of tests ended up being broken, then that would be an indication that > this might be a bigger breaking change for the web than we thought...
any update?
On 2016/10/13 01:55:19, zino wrote: > any update? I would recommend looking at DummyPageHolder and writing a C++ test for this. https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/testing/D... Then you could override the FrameLoaderClient and then capture the loadURLExternally call to validate it was called. Events dispatched to the Page's Event Handler via gmock should all be trusted.
On 2016/10/24 18:48:17, dtapuska wrote: > On 2016/10/13 01:55:19, zino wrote: > > any update? > > I would recommend looking at DummyPageHolder and writing a C++ test for this. > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/testing/D... > > Then you could override the FrameLoaderClient and then capture the > loadURLExternally call to validate it was called. > > Events dispatched to the Page's Event Handler via gmock should all be trusted. Or look at this test here: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/web/tests/TopC... It sends input events to the webview. In https://cs.chromium.org/chromium/src/third_party/WebKit/Source/web/tests/Fram... you could pass your own implementation of https://cs.chromium.org/chromium/src/third_party/WebKit/public/web/WebFrameCl... which is mocked out to be called once or something like that.
dtapuska@chromium.org changed reviewers: + dtapuska@chromium.org |
