Bug 93551 - Some CSS pseudo selectors causing clicks to reload resources on input with type=image
Summary: Some CSS pseudo selectors causing clicks to reload resources on input with ty...
Status: RESOLVED WORKSFORME
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Pravin D
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-08-08 16:44 PDT by Ngan
Modified: 2022-07-13 10:43 PDT (History)
19 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Ngan 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...
Comment 1 Ngan 2012-08-11 04:53:26 PDT
This problem happens on safari 5+ and chrome.
Comment 2 Pravin D 2012-08-17 10:57:47 PDT
Created attachment 159163 [details]
Patch
Comment 3 Daniel Bates 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.
Comment 4 Pravin D 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
Comment 5 Pravin D 2012-08-22 14:01:43 PDT
Created attachment 160006 [details]
Patch
Comment 6 WebKit Review Bot 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
Comment 7 Build Bot 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
Comment 8 Early Warning System Bot 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
Comment 9 Early Warning System Bot 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
Comment 10 Peter Beverloo (cr-android ews) 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
Comment 11 Gyuyoung Kim 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
Comment 12 Gustavo Noronha (kov) 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
Comment 13 Build Bot 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
Comment 14 Pravin D 2012-08-22 20:33:49 PDT
Created attachment 160078 [details]
Patch
Comment 15 Maciej Stachowiak 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.
Comment 16 Pravin D 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
Comment 17 Pravin D 2012-08-29 11:12:30 PDT
Created attachment 161269 [details]
Approach 1
Comment 18 Early Warning System Bot 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
Comment 19 Build Bot 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
Comment 20 WebKit Review Bot 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
Comment 21 Pravin D 2013-02-22 01:43:43 PST
Created attachment 189718 [details]
Patch
Comment 22 Ryosuke Niwa 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?
Comment 23 Pravin D 2013-02-23 01:11:09 PST
Created attachment 189912 [details]
Patch
Comment 24 Pravin D 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.
Comment 25 Antti Koivisto 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).
Comment 26 Antti Koivisto 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.
Comment 27 Andreas Kling 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.
Comment 28 Brent Fulgham 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?