Issue 581 - Fixed element hiding/collapsing in anonymous frames on Chrome

Issue 581 - Fixed element hiding/collapsing in anonymous frames on Chrome

[Wladimir Palant, 2014-05-30 11:45:17]

http://codereview.adblockplus.org/5666188336037888/diff/5629499534213120/incl...
File include.preload.js (right):

http://codereview.adblockplus.org/5666188336037888/diff/5629499534213120/incl...
include.preload.js:55: function checkCollapse(element)
Please move this function and the typeMap declaration back where they originally
were. I cannot review it like this. In general, please always commit (and
review) cosmetic and functional changes separately.

http://codereview.adblockplus.org/5666188336037888/diff/5629499534213120/incl...
include.preload.js:87: function checkExcpetionKey()
Excpetion => Exception

http://codereview.adblockplus.org/5666188336037888/diff/5629499534213120/incl...
include.preload.js:200: if (document.documentElement instanceof HTMLElement)
Why did you remove the failsafe? It was added back when Chrome 18 was the *new*
version, the supported Chrome versions might also show the same behavior.

http://codereview.adblockplus.org/5666188336037888/diff/5629499534213120/incl...
include.preload.js:203: init(document);
Why pass in the document which is a global variable anyway?

[Sebastian Noack, 2014-05-30 12:31:11]

http://codereview.adblockplus.org/5666188336037888/diff/5629499534213120/incl...
File include.preload.js (right):

http://codereview.adblockplus.org/5666188336037888/diff/5629499534213120/incl...
include.preload.js:55: function checkCollapse(element)
On 2014/05/30 11:45:17, Wladimir Palant wrote:
> Please move this function and the typeMap declaration back where they
originally
> were. I cannot review it like this. In general, please always commit (and
> review) cosmetic and functional changes separately.

Done.

http://codereview.adblockplus.org/5666188336037888/diff/5629499534213120/incl...
include.preload.js:87: function checkExcpetionKey()
On 2014/05/30 11:45:17, Wladimir Palant wrote:
> Excpetion => Exception

Done.

http://codereview.adblockplus.org/5666188336037888/diff/5629499534213120/incl...
include.preload.js:200: if (document.documentElement instanceof HTMLElement)
On 2014/05/30 11:45:17, Wladimir Palant wrote:
> Why did you remove the failsafe? It was added back when Chrome 18 was the
*new*
> version, the supported Chrome versions might also show the same behavior.

I tested in Chrome 34 and Chrome 30, and it seems not to be necessary anymore.
Also AdBlock (which claims to be still compatible down to Chrome 23) doesn't do
that (anymore). So I think it is safe to get rid of that check now.

However as discussed on IRC, I don't think that we have to merge this for the
upcoming release, but should heavily test the changes of this patch, regardless
whether we remove that check or not.

http://codereview.adblockplus.org/5666188336037888/diff/5629499534213120/incl...
include.preload.js:203: init(document);
On 2014/05/30 11:45:17, Wladimir Palant wrote:
> Why pass in the document which is a global variable anyway?

It is not always called with the global document. See above, where the
contentDocument of the frame is passed.

[Wladimir Palant, 2014-06-02 11:20:40]

http://codereview.adblockplus.org/5666188336037888/diff/6248215343005696/incl...
File include.preload.js (left):

http://codereview.adblockplus.org/5666188336037888/diff/6248215343005696/incl...
include.preload.js:164: // In Chrome 18 the document might not be initialized
yet
For reference: https://adblockplus.org/forum/viewtopic.php?t=9234 is why the
fallback was introduced. It seems that the issue was reproducible reliable and
it doesn't look like Chromium devs introduced it intentionally. So it was likely
fixed a few versions later.

http://codereview.adblockplus.org/5666188336037888/diff/6248215343005696/incl...
File include.preload.js (right):

http://codereview.adblockplus.org/5666188336037888/diff/6248215343005696/incl...
include.preload.js:66: }
Why read the signature from a tag in Chrome? We read it from the headers, that's
the better approach anyway. And an anonymous frame cannot be expected to have a
signature anyway, only higher-level frames.

http://codereview.adblockplus.org/5666188336037888/diff/6248215343005696/incl...
include.preload.js:194: if (fixAnonymousFrames &&
/^(javascript:|$)/.test(event.target.src))
I think you want to check event.target.contentWindow.location.href, this will
give you a real URL rather than something bogus (e.g. multiple spaces). It will
also throw an exception for frames you aren't allowed to access so you can
ignore those.

The frames that we need to handle here are:

* about:blank
* about:srcdoc
* javascript:* (is that their document URL or rather about:blank?)
* data:*

It is likely that we still have to handle at the very least data:* "manually" in
Chrome 37, this is something you should check.

http://codereview.adblockplus.org/5666188336037888/diff/6248215343005696/incl...
include.preload.js:209: ext.backgroundPage.sendMessage({type: "get-selectors"},
setElemhideCSSRules);
In the background page, who will be the sender of this message in case of an
anonymous frame? Its parent window I guess? That should work correctly then.

What about Chrome 37? Won't the background page see a request coming in from
about:blank URL? That should break element hiding then.

[Sebastian Noack, 2014-06-03 06:55:27]

http://codereview.adblockplus.org/5666188336037888/diff/6248215343005696/incl...
File include.preload.js (right):

http://codereview.adblockplus.org/5666188336037888/diff/6248215343005696/incl...
include.preload.js:66: }
On 2014/06/02 11:20:40, Wladimir Palant wrote:
> Why read the signature from a tag in Chrome? We read it from the headers,
that's
> the better approach anyway. And an anonymous frame cannot be expected to have
a
> signature anyway, only higher-level frames.

This function isn't called for anonymous frames. However that code is used on
Chrome since we have added it for Safari. Nothing new here actually. Feel free
to file a separate issue if you think we must not support that way to provide a
site key on Chrome. But IMO its not worth adding yet another browser switch.

http://codereview.adblockplus.org/5666188336037888/diff/6248215343005696/incl...
include.preload.js:194: if (fixAnonymousFrames &&
/^(javascript:|$)/.test(event.target.src))
On 2014/06/02 11:20:40, Wladimir Palant wrote:
> I think you want to check event.target.contentWindow.location.href, this will
> give you a real URL rather than something bogus (e.g. multiple spaces). It
will
> also throw an exception for frames you aren't allowed to access so you can
> ignore those.

I thought it would be simpler to check for the src attribute. So I don't have to
handle cross-domain-policy violations. But you are right, that approach isn't
really reliable.

> The frames that we need to handle here are:
> 
> * about:blank
> * about:srcdoc

Done.

> * javascript:* (is that their document URL or rather about:blank?)

In case of javascript: URLs, the frame's src attribute returns the given
javascript: URL, while the document's URL will be "about:blank".

> * data:*
> 
> It is likely that we still have to handle at the very least data:* "manually"
in
> Chrome 37, this is something you should check.

Content scripts don't run on data: URLs, not even on Chrome 37 with
"match_about_blank". However the workaround this path implements doesn't work
for data: URLs due to cross-domain-policy.

http://codereview.adblockplus.org/5666188336037888/diff/6248215343005696/incl...
include.preload.js:209: ext.backgroundPage.sendMessage({type: "get-selectors"},
setElemhideCSSRules);
On 2014/06/02 11:20:40, Wladimir Palant wrote:
> In the background page, who will be the sender of this message in case of an
> anonymous frame? Its parent window I guess? That should work correctly then.

Correct.

> What about Chrome 37? Won't the background page see a request coming in from
> about:blank URL? That should break element hiding then.

I can confirm your concern. This also affects Safari, which behaves like Chrome
37 with "match_about_blank". In order to address that, I'm falling back to the
parent's frame URL now, when attempting to get the host of the URL of an
anonymous frame.

[Sebastian Noack, 2014-06-03 07:40:35]

http://codereview.adblockplus.org/5666188336037888/diff/6248215343005696/incl...
File include.preload.js (right):

http://codereview.adblockplus.org/5666188336037888/diff/6248215343005696/incl...
include.preload.js:194: if (fixAnonymousFrames &&
/^(javascript:|$)/.test(event.target.src))
It seems that checking for the document's URL instead frame's src attribute
don't work in cases like:

<iframe onload="document.location.replace(...)">

However this case seems to be pretty common. :(

[Wladimir Palant, 2014-06-04 14:12:48]

http://codereview.adblockplus.org/5666188336037888/diff/5681717746597888/incl...
File include.preload.js (right):

http://codereview.adblockplus.org/5666188336037888/diff/5681717746597888/incl...
include.preload.js:197:
/^\s*(javascript:|about:|$)/i.test(element.getAttribute("src"))))
What about data: URLs? Note that checking whether it is an HTML document is
still necessary for those.

Also, could you check whether/how we need to handle <object data="about:blank"
type="text/html">?

http://codereview.adblockplus.org/5666188336037888/diff/5681717746597888/incl...
include.preload.js:200: init(contentDocument);
Nit: It seems that the contentDocument variable is pointless now.

[Sebastian Noack, 2014-06-04 16:12:30]

http://codereview.adblockplus.org/5666188336037888/diff/5681717746597888/incl...
File include.preload.js (right):

http://codereview.adblockplus.org/5666188336037888/diff/5681717746597888/incl...
include.preload.js:197:
/^\s*(javascript:|about:|$)/i.test(element.getAttribute("src"))))
On 2014/06/04 14:12:49, Wladimir Palant wrote:
> What about data: URLs? Note that checking whether it is an HTML document is
> still necessary for those.

This check intentionally don't match data: URLs, since we can't do anything for
them, due to cross-domain policy.

> Also, could you check whether/how we need to handle <object data="about:blank"
> type="text/html">?

Thanks for pointing this one out. After some testing I can confirm that <object
type="text/html"> elements are handled the same as frames. Content scripts are
only injected when pointing to an external document before Chrome 37. If the
"data" attribute is missing, empty or using either an about: or javascript: URL,
an empty DOM is created which can be accessed via contentDocument.

http://codereview.adblockplus.org/5666188336037888/diff/5681717746597888/incl...
include.preload.js:200: init(contentDocument);
On 2014/06/04 14:12:49, Wladimir Palant wrote:
> Nit: It seems that the contentDocument variable is pointless now.

Done.

[Wladimir Palant, 2014-06-17 13:00:24]

http://codereview.adblockplus.org/5666188336037888/diff/5141887602130944/incl...
File include.preload.js (right):

http://codereview.adblockplus.org/5666188336037888/diff/5141887602130944/incl...
include.preload.js:71: return value == null ||
/^\s*(javascript:|about:|$)/i.test(value);
I'm surprised you check for null explicitly rather than doing .test(value || "")
:)

Doing it this way actually makes sense here, we want to treat null and empty src
attribute the same.

http://codereview.adblockplus.org/5666188336037888/diff/5141887602130944/incl...
include.preload.js:207: element.documentElement))
There is at least one more scenario: <embed src="..." type="text/html">

What is element.documentElement? IMHO this property doesn't exist. I guess you
meant to check element.contentDocument?

The logic here is getting complicated enough that it makes sense to simplify it,
e.g. by using a map:

{
  "frame": src,
  "iframe": src,
  "object": data,
  "embed:" src
}

Then you will still have a special case for srcdoc - that one is specific to the
iframe tag however, it isn't allowed on a frame tag (your code is getting this
wrong right now).

[Sebastian Noack, 2014-06-17 14:13:47]

http://codereview.adblockplus.org/5666188336037888/diff/5141887602130944/incl...
File include.preload.js (right):

http://codereview.adblockplus.org/5666188336037888/diff/5141887602130944/incl...
include.preload.js:71: return value == null ||
/^\s*(javascript:|about:|$)/i.test(value);
On 2014/06/17 13:00:24, Wladimir Palant wrote:
> I'm surprised you check for null explicitly rather than doing .test(value ||
"")
> :)
> 
> Doing it this way actually makes sense here, we want to treat null and empty
src
> attribute the same.

I want to skip regex matching if the attribute is missing, which is the case
with almost all <object> tags. And even if the performance advantage is just
theoretical, I don't see how this adds any complexity.

Also we want to treat javascript: and about: urls the same as an empty string.
But you are not suggesting to replace those with an empty string before testing
the string.

http://codereview.adblockplus.org/5666188336037888/diff/5141887602130944/incl...
include.preload.js:207: element.documentElement))
On 2014/06/17 13:00:24, Wladimir Palant wrote:
> There is at least one more scenario: <embed src="..." type="text/html">

As discussed on IRC, we ignore this case because it is rather exotic and would
add additional complexity since there is no contentDocument property for <embed>
elements.

> What is element.documentElement? IMHO this property doesn't exist. I guess you
> meant to check element.contentDocument?

Ouch, you are right. Fixed.

> The logic here is getting complicated enough that it makes sense to simplify
it,
> e.g. by using a map:
> 
> {
>   "frame": src,
>   "iframe": src,
>   "object": data,
>   "embed:" src
> }

I don't see how that mapping would make anything less complicated. For iframes
we additionally have to check for srcdoc. For objects we have to make sure that
contentDocument exist. And for embed (if we would handle it) we would have to
get the document from window.frames. So beside the attribute there is still one
more thing that is special in each case. However we can move those check into a
separate function using switch/case.

> Then you will still have a special case for srcdoc - that one is specific to
the
> iframe tag however, it isn't allowed on a frame tag (your code is getting this
> wrong right now).

Good point. Fixed.

[Wladimir Palant, 2014-06-17 15:40:53]

LGTM