Issue 2084 - Detect our content script instead checking for anonymous frames

Issue 2084 - Detect our content script instead checking for anonymous frames

[Wladimir Palant, 2015-03-04 19:31:36]

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

http://codereview.adblockplus.org/6677078657204224/diff/5629499534213120/incl...
include.preload.js:87: contentWindow = element.contentWindow;
Accessing element.contentWindow is actually allowed for third-party frames,
otherwise you wouldn't be able to call postMessage() on it for example.

You could use element.contentDocument.defaultView instead, this will also take
care of the check below (this will throw if element.contentDocument is
undefined). I would consider running into an exception here ugly however, so the
other option is keeping the contentDocument variable and merely using
contentDocument.defaultView below. Up to you.

http://codereview.adblockplus.org/6677078657204224/diff/5629499534213120/incl...
include.preload.js:104: return !("isFrameWithoutContentScript" in
contentWindow);
I wonder whether there can be any timing issues here, what if the content script
didn't load yet? I guess you tested that this works correctly?

[Sebastian Noack, 2015-03-04 20:36:13]

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

http://codereview.adblockplus.org/6677078657204224/diff/5629499534213120/incl...
include.preload.js:87: contentWindow = element.contentWindow;
On 2015/03/04 19:31:36, Wladimir Palant wrote:
> Accessing element.contentWindow is actually allowed for third-party frames,
> otherwise you wouldn't be able to call postMessage() on it for example.
> 
> You could use element.contentDocument.defaultView instead, this will also take
> care of the check below (this will throw if element.contentDocument is
> undefined). I would consider running into an exception here ugly however, so
the
> other option is keeping the contentDocument variable and merely using
> contentDocument.defaultView below. Up to you.

You are right. Also I realized that we can actually access third-party frames.
Even contentWindow.isFrameWithoutContentScript won't raise an exception, but
contentDocument does. Moreover, I realized that <object> elements only have
contentDocument, but no contentDocument anyway.

http://codereview.adblockplus.org/6677078657204224/diff/5629499534213120/incl...
include.preload.js:104: return !("isFrameWithoutContentScript" in
contentWindow);
On 2015/03/04 19:31:36, Wladimir Palant wrote:
> I wonder whether there can be any timing issues here, what if the content
script
> didn't load yet? I guess you tested that this works correctly?

Yes, I tested it on a few websites of course, but timing issues aren't usually
reliably reproducible. However, we run this content script at "document_start",
but this check is performed on "load" event, so we should be safe.

For reference:
In the case of "document_start", the files are injected after any files from
css, but before any other DOM is constructed or any other script is run.

[Wladimir Palant, 2015-03-04 21:17:41]

LGTM