Issue 5382 - Wrap DOM mutation APIs to protect frames

Issue 5382 - Wrap DOM mutation APIs to protect frames

[Manish Jethani, 2017-10-23 22:17:12]

Patch Set 1

The problem with #5382 is that you can access the frame's in a document simply
using their name or index (e.g. window["my-frame"] or window[12]).

I've thought about a few approaches:

 1.  Use a MutationObserver: It doesn't give you the events in time, the author
already has the content window by then
 2.  Use "DOMNodeInserted": Too slow apparently, deprecated, will cause
performance problems
 3.  Wrap the APIs that mutate the DOM and try to wrap all the frames each time

I'm doing the third one here.

Now I don't know about performance, I can check it out by testing on the major
sites.

We could just get rid of the contentWindow and contentDocument wrappers and just
preemptively inject our code into any frame that's added to the document.

[Manish Jethani, 2017-10-24 01:43:29]

Patch Set 2: Refactor

https://codereview.adblockplus.org/29586710/diff/29586720/inject.preload.js
File inject.preload.js (right):

https://codereview.adblockplus.org/29586710/diff/29586720/inject.preload.js#n...
inject.preload.js:106: wrapPropertyAPIForInjection(HTMLObjectElement.prototype,
HTMLFrameElement and HTMLIFrameElement don't need wrappers now because we're
injecting the code preemptively.

For HTMLObjectElement we don't have to worry about Chrome 42, it's too old now.

[kzar, 2017-12-11 08:48:14]

Sorry for being so slow with this review.

I'm concerned about adding more here since content scripts seem to be a
performance problem in Firefox (#6067) possibly since they are run for all tabs
when the extension is loaded. What do you think?

https://codereview.adblockplus.org/29586710/diff/29586720/inject.preload.js
File inject.preload.js (right):

https://codereview.adblockplus.org/29586710/diff/29586720/inject.preload.js#n...
inject.preload.js:86: let returnValue = func.apply(this, args);
I think func.apply could have been messed with at this point. I recommend
reading through all Tartpvule's comments[1] on the similar codereviews to get an
idea about some of the things we have to watch out for. There's likely some
other similar problems.

[1] -
https://codereview.adblockplus.org/search?closed=1&owner=&reviewer=chinrtgm1%...

https://codereview.adblockplus.org/29586710/diff/29586720/inject.preload.js#n...
inject.preload.js:106: wrapPropertyAPIForInjection(HTMLObjectElement.prototype,
On 2017/10/24 01:43:28, Manish Jethani wrote:
> HTMLFrameElement and HTMLIFrameElement don't need wrappers now because we're
> injecting the code preemptively.
> 
> For HTMLObjectElement we don't have to worry about Chrome 42, it's too old
now.

Fair enough but did you check that it's OK with Chrome 49?

[Manish Jethani, 2018-02-21 16:37:39]

On 2017/12/11 08:48:14, kzar wrote:
> Sorry for being so slow with this review.
> 
> I'm concerned about adding more here since content scripts seem to be a
> performance problem in Firefox (#6067) possibly since they are run for all
tabs
> when the extension is loaded. What do you think?

It's a tradeoff, I'm concerned about this slowing down web pages, I think I'll
try to run some tests to see what the actual impact of this is.

https://codereview.adblockplus.org/29586710/diff/29586720/inject.preload.js
File inject.preload.js (right):

https://codereview.adblockplus.org/29586710/diff/29586720/inject.preload.js#n...
inject.preload.js:86: let returnValue = func.apply(this, args);
On 2017/12/11 08:48:13, kzar wrote:
> I think func.apply could have been messed with at this point. I recommend
> reading through all Tartpvule's comments[1] on the similar codereviews to get
an
> idea about some of the things we have to watch out for. There's likely some
> other similar problems.
> 
> [1] -
>
https://codereview.adblockplus.org/search?closed=1&owner=&reviewer=chinrtgm1%...

Thanks, for this one in particular now I've bound the function to
Function.prototype.apply similar to what we're doing in other places.

https://codereview.adblockplus.org/29586710/diff/29586720/inject.preload.js#n...
inject.preload.js:106: wrapPropertyAPIForInjection(HTMLObjectElement.prototype,
On 2017/12/11 08:48:13, kzar wrote:
> On 2017/10/24 01:43:28, Manish Jethani wrote:
> > HTMLFrameElement and HTMLIFrameElement don't need wrappers now because we're
> > injecting the code preemptively.
> > 
> > For HTMLObjectElement we don't have to worry about Chrome 42, it's too old
> now.
> 
> Fair enough but did you check that it's OK with Chrome 49?

I've added a check now so if the descriptor is not found it'll just return.

[Manish Jethani, 2018-02-21 16:38:15]

Patch Set 4

[Manish Jethani, 2018-06-12 08:54:13]

Since Snippets is so close to getting developed and released, we could move our
API wrappers, including this strengthening, to the snippets library. This way it
won't affect all web document, only those that it has been injected into, and we
can be a lot less concerned about performance.

I will close this now.