Issue 6317759025643520: Issue 1396 - Avoid infinite recursion when collapsing <input type="image"> elements (Closed)
|
Created:
Sept. 24, 2014, 11:05 a.m. by Sebastian Noack Modified:
Sept. 25, 2014, 6:52 a.m. Visibility:
Public. |
DescriptionIssue 1396 - Avoid infinite recursion when collapsing <input tyoe="image"> elements
Patch Set 1 : #
Total comments: 7
Patch Set 2 : Addressed comment #MessagesTotal messages: 9
http://codereview.adblockplus.org/6317759025643520/diff/5668600916475904/incl... File include.preload.js (right): http://codereview.adblockplus.org/6317759025643520/diff/5668600916475904/incl... include.preload.js:57: element._collapsing = true; This makes us quite easily distinguishable from other adblockers. What about using WeakSet for browsers that support it? Apart from that, this is an HTMLElement so I'd rather use `element.dataset.collapsed = true;` to keep it standards conformant.
http://codereview.adblockplus.org/6317759025643520/diff/5668600916475904/incl... File include.preload.js (right): http://codereview.adblockplus.org/6317759025643520/diff/5668600916475904/incl... include.preload.js:57: element._collapsing = true; On 2014/09/24 12:35:32, Thomas Greiner wrote: > This makes us quite easily distinguishable from other adblockers. What about > using WeakSet for browsers that support it? > > Apart from that, this is an HTMLElement so I'd rather use > `element.dataset.collapsed = true;` to keep it standards conformant. No, it doesn't. Content scripts run in a different universe. Custom properties we add to objects aren't visible to the website. However, if we change DOM content (that includes using the dataset property) it will be visible to the website.
LGTM http://codereview.adblockplus.org/6317759025643520/diff/5668600916475904/incl... File include.preload.js (right): http://codereview.adblockplus.org/6317759025643520/diff/5668600916475904/incl... include.preload.js:57: element._collapsing = true; On 2014/09/24 12:50:09, Sebastian Noack wrote: > On 2014/09/24 12:35:32, Thomas Greiner wrote: > > This makes us quite easily distinguishable from other adblockers. What about > > using WeakSet for browsers that support it? > > > > Apart from that, this is an HTMLElement so I'd rather use > > `element.dataset.collapsed = true;` to keep it standards conformant. > > No, it doesn't. Content scripts run in a different universe. Custom properties > we add to objects aren't visible to the website. However, if we change DOM > content (that includes using the dataset property) it will be visible to the > website. I can confirm that custom properties are not part of the shared DOM.
http://codereview.adblockplus.org/6317759025643520/diff/5668600916475904/incl... File include.preload.js (right): http://codereview.adblockplus.org/6317759025643520/diff/5668600916475904/incl... include.preload.js:36: return; Interesting, a load triggered for an element with display:none sounds like a bug. I could confirm that this indeed happens: only for <input type="image"> but not for <img>, and only if the request was blocked (maybe pointing the request to a non-existing server will have the same effect). I doubt strongly that other browsers like Firefox behave the same, sounds worth filing a Chrome bug on this. Either way, the name of that variable is misleading - this isn't an re-entrance check, element._collapsing is never removed. It rather marks the elements as "already collapsed, don't do anything with it." While scenarios where not collapsing an element a second time will hurt us should be rather rare, IMHO there is a much simpler and more robust way to avoid infinite recursion: don't set the element.style.display if its value is already "none."
http://codereview.adblockplus.org/6317759025643520/diff/5668600916475904/incl... File include.preload.js (right): http://codereview.adblockplus.org/6317759025643520/diff/5668600916475904/incl... include.preload.js:36: return; On 2014/09/24 14:57:00, Wladimir Palant wrote: > maybe pointing the request > to a non-existing server will have the same effect That's what I checked before and it does have the same effect indeed. Unfortunately, the W3C standard doesn't really define a behavior for this case only "if the fetching process fails without a response from the remote server, or completes but the image is not a valid or supported image" (see http://www.w3.org/TR/2012/WD-html5-20121025/states-of-the-type-attribute.html...).
http://codereview.adblockplus.org/6317759025643520/diff/5668600916475904/incl... File include.preload.js (right): http://codereview.adblockplus.org/6317759025643520/diff/5668600916475904/incl... include.preload.js:36: return; On 2014/09/24 15:08:29, Thomas Greiner wrote: > Unfortunately, the W3C standard doesn't really define a behavior for this case > only "if the fetching process fails without a response from the remote server, > or completes but the image is not a valid or supported image" (see > http://www.w3.org/TR/2012/WD-html5-20121025/states-of-the-type-attribute.html.... The only thing the standard defines for that particular case is that an "error" event must be dispatched. However, the standard also specifies when the image must be fetched. But allows to ignore those rules and fetch the image on demand instead, which isn't further specified. Also it doesn't explicitly forbids to re-fetch images later. So from what I can tell, Chrome's behavior should be fine.
http://codereview.adblockplus.org/6317759025643520/diff/5668600916475904/incl... File include.preload.js (right): http://codereview.adblockplus.org/6317759025643520/diff/5668600916475904/incl... include.preload.js:36: return; On 2014/09/24 14:57:00, Wladimir Palant wrote: > check, element._collapsing is never removed. It rather marks the elements as > "already collapsed, don't do anything with it." While scenarios where not > collapsing an element a second time will hurt us should be rather rare, IMHO > there is a much simpler and more robust way to avoid infinite recursion: don't > set the element.style.display if its value is already "none." Done, though it wasn't actually that simply. I also have to check for the priority. Otherwise collapsing won't work if the "style" attribute already specifies "display: none" and an external stylesheet overrides it with "!important".
LGTM 
|
