Avoid doc.write if a 3p frame has already been instrumented

[lannka]

Fixes #9249

@cramforce , do you remember why we had the instrumentDocWrite code?

doc.write('<script>window.parent.ampManageWin(window)</script>'); is making ads blank.
(we might override doc.close too late?)

[jridgewell]

Bad ads installing video stream decoding JS.

[lannka]

@jridgewell why manageWin is not enough and we have to instrumentDocWrite?

[jridgewell]

http://stackoverflow.com/questions/3216871/why-does-using-document-write-to-create-an-iframe-cancel-out-all-remaining-jav

[lannka]

@jridgewell don't quite understand your point.

[jridgewell]

It's another defense against badly written code.

[lannka]

the mystery to me is that we already did manageWin(iframe.contentWindow) immediately when we saw a new iframe window. but for some reasons, we did the same thing again and again at different time:

1. iframe.onload = function () { /* one time here */}
2. iframe.contentWindow.document.close = function () { /* one more time here */}

In my local test, I found 1) is necessary, though I don't have explanation. In fact, when I try to debug, it becomes unnecessary.
2) seems to be not needed, or maybe I wasn't using an appropriate test case.

So really want to hear from @cramforce who introduced those hacks :-). What problems were there?

[dvoytenko]

DO NOT SUBMIT?

[dvoytenko]

@lannka Do we know the reason why it breaks anything, however?

[cramforce]

Are we running the code twice on the same window. There is a case of removing all of this code now that browsers throttle themselves more aggressively.

[lannka]

@lannka Do we know the reason why it breaks anything, however?

@dvoytenko as I mentioned in PR description

doc.write('<script>window.parent.ampManageWin(window)</script>'); is making ads blank.
(we might override doc.close too late?)

See that screenshot.

Are we running the code twice on the same window. There is a case of removing all of this code now that browsers throttle themselves more aggressively.

@cramforce No. manageWin() is flag protected (window.ampSeen). However, the doc.write is not. I can do

if (!parent.ampSeen) {
  doc.write('<script>window.parent.ampManageWin(window)</script>');		
}

But I want to understand what corner case this code is trying to handle, can we get rid of them completely? At least the doc.write part, which seems dangerous.

[cramforce]

That conditional should definitely be added!!

There are many ways how iframes are added to ad pages. This is handling one of those cases. Might or might not be needed for doubleclick.

[lannka]

@cramforce adding the check fixes the issue. this is good for now, let's follow up offline see if we can improve some of the code here.

[cramforce]

Code is cool, but the PR description doesn't correctly reflect it.

[lannka]

updated description