Bug 93551

Summary:

Some CSS pseudo selectors causing clicks to reload resources on input with type=image

Product:

WebKit

Reporter:

Ngan <nganpham>

Component:

CSS

Assignee:

Pravin D <pravind.2k4>

Status:

RESOLVED WORKSFORME

Severity:

Normal

CC:

ap, bfulgham, darin, dbates, dglazkov, esprehn+autocc, gustavo, kling, koivisto, mifenton, nganpham, ojan.autocc, peter+ews, pravind.2k4, pravind, rniwa, tkent, webkit.review.bot, xan.lopez

Priority:

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Approach 1	none
Patch	none
Patch	none

Ngan

Reported 2012-08-08 16:44:00 PDT

This is a very strange bug, so please bare with me... I have the following html document: <!DOCTYPE html> <html> <head> <style> :active { } </style> </head> <body> <div style="display: none"> <input type="image" src="http://localhost:3000/asdf"> </div> </body> </html> I have a server running on 3000 that simple tells me when it's getting hit. If you open that html document, and click ANYWHERE on the page, the browser will make a request to that src URL over and over again for every click. What I know: - The input element must be hidden (the div with display none does that). - The CSS must be a pseudo selector (:active works, others might too). - The CSS selector doesn't even have to match! (e.g. .asdf :active { }) will cause the bug as well. - The issue only happens when the element is an input type="image" element (for all I know). You might not need a server to test this. You can just check out the timeline and see requests being made as well...

Attachments
Patch (5.67 KB, patch) 2012-08-17 10:57 PDT, Pravin D	no flags	Details Formatted Diff Diff
Patch (5.63 KB, patch) 2012-08-22 14:01 PDT, Pravin D	no flags	Details Formatted Diff Diff
Patch (5.61 KB, patch) 2012-08-22 20:33 PDT, Pravin D	no flags	Details Formatted Diff Diff
Approach 1 (5.69 KB, patch) 2012-08-29 11:12 PDT, Pravin D	no flags	Details Formatted Diff Diff
Patch (10.63 KB, patch) 2013-02-22 01:43 PST, Pravin D	no flags	Details Formatted Diff Diff
Patch (11.96 KB, patch) 2013-02-23 01:11 PST, Pravin D	no flags	Details Formatted Diff Diff
Show Obsolete (5) View All Add attachment proposed patch, testcase, etc.

Ngan

Comment 1 2012-08-11 04:53:26 PDT

This problem happens on safari 5+ and chrome.

Pravin D

Comment 2 2012-08-17 10:57:47 PDT

Created attachment 159163 [details] Patch

Daniel Bates

Comment 3 2012-08-22 13:25:44 PDT

Comment on attachment 159163 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=159163&action=review I did not review this patch for correctness. > Source/WebCore/ChangeLog:10 > + On mouseClick event on a page having image type input element with universal pseudo selector ":active" > + should not send any network requests for the associated image resource for the input element > + when its effective(either elements style or its ancestor) display is none. Another way to phrase this sentence is: Mouse click events on a page that defines the universal pseudo selector :active, and has an <input type="image"> element E where E or its ancestor has display none should not initiate an HTTP request for the image resource associated with E. OR Fixes an issue where every mouse click on a specially crafted page that has an <input type="image"> element E initiates an HTTP request for the image resource associated with E. Consider a page that defines the universal pseudo selector :active, has an <input type="image"> element E, and either E or its ancestor has style display none. Every mouse click on the page initiates an HTTP request for the image resource associated with E. > Source/WebCore/ChangeLog:16 > + When a new imageLoader is created resource request is sent so that it can be cached. However subsequent Nit: "When a new imageLoader is created resource..." => "When a new ImageLoader is created a resource..." > LayoutTests/http/tests/misc/image-type-input-with-display-none-resource-request.html:7 > +<!-- > +Description: On mouseClick event on a page having image type input element with universal pseudo selector ":active" > + should not send any network requests for the associated image resource for the input element Ditto.

Pravin D

Comment 4 2012-08-22 13:32:06 PDT

(In reply to comment #3) > (From update of attachment 159163 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=159163&action=review > > I did not review this patch for correctness. > > > Source/WebCore/ChangeLog:10 > > + On mouseClick event on a page having image type input element with universal pseudo selector ":active" > > + should not send any network requests for the associated image resource for the input element > > + when its effective(either elements style or its ancestor) display is none. > > Another way to phrase this sentence is: > > Mouse click events on a page that defines the universal pseudo selector :active, and has an <input type="image"> element E where E or its ancestor has display none should not initiate an HTTP request for the image resource associated with E. > > OR > > Fixes an issue where every mouse click on a specially crafted page that has an <input type="image"> element E initiates an HTTP request for the image resource associated with E. > > Consider a page that defines the universal pseudo selector :active, has an <input type="image"> element E, and either E or its ancestor has style display none. Every mouse click on the page initiates an HTTP request for the image resource associated with E. > > > Source/WebCore/ChangeLog:16 > > + When a new imageLoader is created resource request is sent so that it can be cached. However subsequent > Would like to go with the 1st of the suggestions that you have given. > Nit: "When a new imageLoader is created resource..." => "When a new ImageLoader is created a resource..." > > > LayoutTests/http/tests/misc/image-type-input-with-display-none-resource-request.html:7 > > +<!-- > > +Description: On mouseClick event on a page having image type input element with universal pseudo selector ":active" > > + should not send any network requests for the associated image resource for the input element > > Ditto. Will do it. Thnks for the inputs Daniel

Pravin D

Comment 5 2012-08-22 14:01:43 PDT

Created attachment 160006 [details] Patch

WebKit Review Bot

Comment 6 2012-08-22 14:15:42 PDT

Comment on attachment 160006 [details] Patch Attachment 160006 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13563407

Build Bot

Comment 7 2012-08-22 14:24:46 PDT

Comment on attachment 160006 [details] Patch Attachment 160006 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13564405

Early Warning System Bot

Comment 8 2012-08-22 14:36:46 PDT

Comment on attachment 160006 [details] Patch Attachment 160006 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/13569382

Early Warning System Bot

Comment 9 2012-08-22 14:48:12 PDT

Comment on attachment 160006 [details] Patch Attachment 160006 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/13567389

Peter Beverloo (cr-android ews)

Comment 10 2012-08-22 15:10:48 PDT

Comment on attachment 160006 [details] Patch Attachment 160006 [details] did not pass cr-android-ews (chromium-android): Output: http://queues.webkit.org/results/13568391

Gyuyoung Kim

Comment 11 2012-08-22 15:41:38 PDT

Comment on attachment 160006 [details] Patch Attachment 160006 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/13559451

Gustavo Noronha (kov)

Comment 12 2012-08-22 17:10:51 PDT

Comment on attachment 160006 [details] Patch Attachment 160006 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/13560451

Build Bot

Comment 13 2012-08-22 17:55:48 PDT

Comment on attachment 160006 [details] Patch Attachment 160006 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13564480

Pravin D

Comment 14 2012-08-22 20:33:49 PDT

Created attachment 160078 [details] Patch

Maciej Stachowiak

Comment 15 2012-08-25 00:31:23 PDT

Comment on attachment 160078 [details] Patch This change looks wrong. It looks like it will still call m_imageLoader->updateFromElement unnecessarily in other cases. It also makes the logic hard to follow. Instead of this, you should look at what HTMLImageElement does. It does not call updateFromElement() in attach() at all, only when setting the src attribute or when inserting into document, and then only if the image loader had not loaded an image already. That is much better than the somewhat kludgey change here.

Pravin D

Comment 16 2012-08-25 08:54:15 PDT

(In reply to comment #15) > (From update of attachment 160078 [details]) > This change looks wrong. It looks like it will still call m_imageLoader->updateFromElement unnecessarily in other cases. It also makes the logic hard to follow. Instead of this, you should look at what HTMLImageElement does. It does not call updateFromElement() in attach() at all, only when setting the src attribute or when inserting into document, and then only if the image loader had not loaded an image already. That is much better than the somewhat kludgey change here. > Was trying to make minimal changes. I guess it was not such a good idea. I upload another patch with suggested approach. Thnks for the review

Pravin D

Comment 17 2012-08-29 11:12:30 PDT

Created attachment 161269 [details] Approach 1

Early Warning System Bot

Comment 18 2012-08-29 12:04:36 PDT

Comment on attachment 161269 [details] Approach 1 Attachment 161269 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/13701004

Build Bot

Comment 19 2012-08-29 13:40:11 PDT

Comment on attachment 161269 [details] Approach 1 Attachment 161269 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13690337

WebKit Review Bot

Comment 20 2012-08-29 21:14:00 PDT

Comment on attachment 161269 [details] Approach 1 Attachment 161269 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13689533 New failing tests: fast/forms/input-valueasnumber-unsupported.html fast/forms/input-type-change3.html fast/forms/fieldset/fieldset-elements.html fast/forms/input-type-change.html fast/forms/ValidityState-stepMismatch.html fast/forms/radionodelist-image-type.html fast/forms/input-image-submit.html fast/forms/input-width-height-attributes.html

Pravin D

Comment 21 2013-02-22 01:43:43 PST

Created attachment 189718 [details] Patch

Ryosuke Niwa

Comment 22 2013-02-22 12:03:52 PST

Comment on attachment 189718 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=189718&action=review > LayoutTests/http/tests/misc/image-type-input-with-display-none-resource-request.html:9 > +Description: Mouse click events on a page that contains a universal selector :active, and has an <input type="image"> element E > + should not initiate a HTTP request for the image resource associated with E where E or its ancestor has display:none. > + > +Expected: The last entry in the network log should be PASS. The associated expected file will have just one entry "PASS" This should be included in the expected result so that people looking at the expected result can tell what's happening in the test. > LayoutTests/http/tests/misc/image-type-input-with-display-none-resource-request.html:16 > +function CallCommand(cmd, shouldEndTest) { CallCommand is a very vague name. > LayoutTests/http/tests/misc/image-type-input-with-display-none-resource-request.html:21 > + req.onreadystatechange=function() { Spaces are needed around =. > LayoutTests/http/tests/misc/image-type-input-with-display-none-resource-request.html:22 > + if (req.readyState==4 && req.status==200) { Wrong indentation. Spaces are needed around ==. > LayoutTests/http/tests/misc/image-type-input-with-display-none-resource-request.html:54 > + setTimeout(sendMouseEvents, 10); Why are we waiting for 10ms? > LayoutTests/http/tests/misc/image-type-input-with-display-none-resource-request.html:57 > +function endTest(e) { "e" is never used here. > LayoutTests/http/tests/misc/image-type-input-with-display-none-resource-request.html:58 > + setTimeout( function() { CallCommand("get-resource-request-log"); }, 100); Why are we waiting for 100ms?

Pravin D

Comment 23 2013-02-23 01:11:09 PST

Created attachment 189912 [details] Patch

Pravin D

Comment 24 2013-02-23 03:06:03 PST

(In reply to comment #22) > (From update of attachment 189718 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=189718&action=review > > > LayoutTests/http/tests/misc/image-type-input-with-display-none-resource-request.html:9 > > +Description: Mouse click events on a page that contains a universal selector :active, and has an <input type="image"> element E > > + should not initiate a HTTP request for the image resource associated with E where E or its ancestor has display:none. > > + > > +Expected: The last entry in the network log should be PASS. The associated expected file will have just one entry "PASS" > > This should be included in the expected result so that people looking at the expected result can tell what's happening in the test. > Done. > > LayoutTests/http/tests/misc/image-type-input-with-display-none-resource-request.html:16 > > +function CallCommand(cmd, shouldEndTest) { > > CallCommand is a very vague name. > Changed the name to networkRequestLogCommand(). > > LayoutTests/http/tests/misc/image-type-input-with-display-none-resource-request.html:21 > > + req.onreadystatechange=function() { > > Spaces are needed around =. > > > LayoutTests/http/tests/misc/image-type-input-with-display-none-resource-request.html:22 > > + if (req.readyState==4 && req.status==200) { > > Wrong indentation. Spaces are needed around ==. > Done > > LayoutTests/http/tests/misc/image-type-input-with-display-none-resource-request.html:54 > > + setTimeout(sendMouseEvents, 10); > > Why are we waiting for 10ms? > When the src of the element is changed it takes at least 2ms for the resource request to be made to the server. We want to send mouse events only after this request is made. To be on the safer side made the wait time to 10ms. > > LayoutTests/http/tests/misc/image-type-input-with-display-none-resource-request.html:57 > > +function endTest(e) { > > "e" is never used here. > Removed "e". > > LayoutTests/http/tests/misc/image-type-input-with-display-none-resource-request.html:58 > > + setTimeout( function() { CallCommand("get-resource-request-log"); }, 100); > > Why are we waiting for 100ms? > This was not need. Removed the same.

Antti Koivisto

Comment 25 2013-02-27 12:31:48 PST

So as far as I see there are at least two major bugs here: - :active { } is not a no-op - <input type="image"> that is a child of display:none element is getting attached I think these should be investigated. Matching the img element loading behavior here seems like something that just hides these bugs (and it is unclear if it is desirable or not).

Antti Koivisto

Comment 26 2013-02-27 12:42:01 PST

Generally we try to load only resources that are actually needed. We do load all <img>'s for performance and legacy reasons but for example css backgrounds are only loaded on demand (on style resolve time). It is not clear to me we should change <input type=image> behavior like this.

Andreas Kling

Comment 27 2014-02-05 11:12:37 PST

Comment on attachment 189912 [details] Patch Clearing review flag on patches from before 2014. If this patch is still relevant, please reset the r? flag.

Brent Fulgham

Comment 28 2022-07-13 10:43:58 PDT

We don't believe there is an active issue here. Can you please REOPEN this bug if you believe there is still a concern?

Note You need to log in before you can comment on or make changes to this bug.