From 765c5a7e9ae5891f54ec8ecda499c9a21d7d3c87 Mon Sep 17 00:00:00 2001 From: Justin Ridgewell Date: Tue, 19 Apr 2016 15:57:25 -0700 Subject: [PATCH 1/4] Forbid adding event listeners on appended iframes Fixes https://github.com/ampproject/amphtml/issues/2944. --- src/iframe-helper.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/iframe-helper.js b/src/iframe-helper.js index 23aefae011a2..b4f3bb7dabf6 100644 --- a/src/iframe-helper.js +++ b/src/iframe-helper.js @@ -31,6 +31,8 @@ import {parseUrl} from './url'; */ export function listen(iframe, typeOfMessage, callback, opt_is3P) { dev.assert(iframe.src, 'only iframes with src supported'); + dev.assert(!iframe.parentNode, 'cannot register events on an iframe inside ' + + 'the DOM tree. It will cause hair-pulling bugs like #2942'); const origin = parseUrl(iframe.src).origin; let win = iframe.ownerDocument.defaultView; const sentinel = getSentinel_(opt_is3P); From 97b94727c8fda02ab24c1f2900a0e3200f76bb4f Mon Sep 17 00:00:00 2001 From: Justin Ridgewell Date: Tue, 19 Apr 2016 16:08:16 -0700 Subject: [PATCH 2/4] tests --- src/iframe-helper.js | 4 +-- test/fixtures/served/iframe-intersection.html | 10 +++---- test/functional/test-iframe-helper.js | 27 ++++++++++++++++--- 3 files changed, 30 insertions(+), 11 deletions(-) diff --git a/src/iframe-helper.js b/src/iframe-helper.js index b4f3bb7dabf6..d986889956be 100644 --- a/src/iframe-helper.js +++ b/src/iframe-helper.js @@ -31,8 +31,8 @@ import {parseUrl} from './url'; */ export function listen(iframe, typeOfMessage, callback, opt_is3P) { dev.assert(iframe.src, 'only iframes with src supported'); - dev.assert(!iframe.parentNode, 'cannot register events on an iframe inside ' + - 'the DOM tree. It will cause hair-pulling bugs like #2942'); + dev.assert(!iframe.parentNode, 'cannot register events on an attached ' + + 'iframe. It will cause hair-pulling bugs like #2942'); const origin = parseUrl(iframe.src).origin; let win = iframe.ownerDocument.defaultView; const sentinel = getSentinel_(opt_is3P); diff --git a/test/fixtures/served/iframe-intersection.html b/test/fixtures/served/iframe-intersection.html index c749be94edb4..2aa8b57ae16b 100644 --- a/test/fixtures/served/iframe-intersection.html +++ b/test/fixtures/served/iframe-intersection.html @@ -12,12 +12,10 @@ el.value = el.value + '\n[' + (new Date().getTime() - startTime) + '] ' + (m || ''); } -window.setTimeout(function() { - parent./*OK*/postMessage({ - sentinel: 'amp', - type: 'send-intersections' - }, '*'); -}, 1000); +parent./*OK*/postMessage({ + sentinel: 'amp', + type: 'send-intersections' +}, '*'); window.addEventListener('message', function(event) { if (event.data) { diff --git a/test/functional/test-iframe-helper.js b/test/functional/test-iframe-helper.js index d282960af8e4..8996f87c00b1 100644 --- a/test/functional/test-iframe-helper.js +++ b/test/functional/test-iframe-helper.js @@ -26,13 +26,16 @@ describe('iframe-helper', function() { let sandbox; let container; + function insert(iframe) { + container.doc.body.appendChild(iframe); + } + beforeEach(() => { sandbox = sinon.sandbox.create(); return createIframePromise().then(c => { container = c; - const i = document.createElement('iframe'); + const i = c.doc.createElement('iframe'); i.src = iframeSrc; - container.doc.body.appendChild(i); testIframe = i; }); }); @@ -42,6 +45,22 @@ describe('iframe-helper', function() { sandbox.restore(); }); + it('should assert src in iframe', () => { + const iframe = container.doc.createElement('iframe'); + iframe.srcdoc = ''; + expect(() => { + IframeHelper.listen(iframe, 'test', () => {}); + }).to.throw('only iframes with src supported'); + }); + + it('should assert iframe is detached', () => { + const iframe = container.doc.createElement('iframe'); + iframe.src = iframeSrc; + insert(iframe); + expect(() => { + IframeHelper.listen(iframe, 'test', () => {}); + }).to.throw('cannot register events on an attached iframe'); + }); it('should listen to iframe messages', () => { const removeEventListenerSpy = sandbox.spy(container.win, @@ -50,6 +69,7 @@ describe('iframe-helper', function() { return new Promise(resolve => { unlisten = IframeHelper.listen(testIframe, 'send-intersections', resolve); + insert(testIframe); }).then(() => { expect(removeEventListenerSpy.callCount).to.equal(0); unlisten(); @@ -62,6 +82,7 @@ describe('iframe-helper', function() { 'removeEventListener'); return new Promise(resolve => { IframeHelper.listenOnce(testIframe, 'send-intersections', resolve); + insert(testIframe); }).then(() => { expect(removeEventListenerSpy.callCount).to.equal(1); }); @@ -71,7 +92,6 @@ describe('iframe-helper', function() { const removeEventListenerSpy = sandbox.spy(container.win, 'removeEventListener'); IframeHelper.listen(testIframe, 'send-intersections', function() {}); - testIframe.parentElement.removeChild(testIframe); expect(removeEventListenerSpy.callCount).to.equal(0); container.win.postMessage('hello world', '*'); expect(removeEventListenerSpy.callCount).to.equal(0); @@ -81,6 +101,7 @@ describe('iframe-helper', function() { }); it('should set sentinel on postMessage data', () => { + insert(testIframe); postMessageSpy = sinon/*OK*/.spy(testIframe.contentWindow, 'postMessage'); IframeHelper.postMessage( testIframe, 'testMessage', {}, 'http://google.com'); From c7128088c27e982142043faa5e6d0c665eaa0bd0 Mon Sep 17 00:00:00 2001 From: Justin Ridgewell Date: Tue, 19 Apr 2016 17:11:34 -0700 Subject: [PATCH 3/4] Fix other listens on attached iframe --- extensions/amp-facebook/0.1/amp-facebook.js | 2 +- extensions/amp-iframe/0.1/amp-iframe.js | 4 ++-- extensions/amp-twitter/0.1/amp-twitter.js | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/extensions/amp-facebook/0.1/amp-facebook.js b/extensions/amp-facebook/0.1/amp-facebook.js index 723826b0e7c0..851aa2a5e225 100644 --- a/extensions/amp-facebook/0.1/amp-facebook.js +++ b/extensions/amp-facebook/0.1/amp-facebook.js @@ -41,7 +41,6 @@ class AmpFacebook extends AMP.BaseElement { const iframe = getIframe(this.element.ownerDocument.defaultView, this.element, 'facebook'); this.applyFillContent(iframe); - this.element.appendChild(iframe); // Triggered by context.updateDimensions() inside the iframe. listen(iframe, 'embed-size', data => { iframe.height = data.height; @@ -51,6 +50,7 @@ class AmpFacebook extends AMP.BaseElement { amp.setAttribute('width', data.width); this./*OK*/changeHeight(data.height); }, /* opt_is3P */true); + this.element.appendChild(iframe); return loadPromise(iframe); } }; diff --git a/extensions/amp-iframe/0.1/amp-iframe.js b/extensions/amp-iframe/0.1/amp-iframe.js index bcd7b5f7eb92..9ff280a9b407 100644 --- a/extensions/amp-iframe/0.1/amp-iframe.js +++ b/extensions/amp-iframe/0.1/amp-iframe.js @@ -258,8 +258,6 @@ export class AmpIframe extends AMP.BaseElement { setSandbox(this.element, iframe, this.sandbox_); iframe.src = this.iframeSrc; - this.container_.appendChild(iframe); - if (!isTracking) { this.intersectionObserver_ = new IntersectionObserver(this, iframe); } @@ -306,6 +304,8 @@ export class AmpIframe extends AMP.BaseElement { listen(iframe, 'embed-ready', this.activateIframe_.bind(this)); } + this.container_.appendChild(iframe); + return loadPromise(iframe).then(() => { // On iOS the iframe at times fails to render inside the `overflow:auto` // container. To avoid this problem, we set the `overflow:auto` property diff --git a/extensions/amp-twitter/0.1/amp-twitter.js b/extensions/amp-twitter/0.1/amp-twitter.js index 6d3b4c71e165..79ce58c1ca57 100644 --- a/extensions/amp-twitter/0.1/amp-twitter.js +++ b/extensions/amp-twitter/0.1/amp-twitter.js @@ -50,7 +50,6 @@ class AmpTwitter extends AMP.BaseElement { const iframe = getIframe(this.element.ownerDocument.defaultView, this.element, 'twitter'); this.applyFillContent(iframe); - this.element.appendChild(iframe); // Triggered by context.updateDimensions() inside the iframe. listen(iframe, 'embed-size', data => { // We only get the message if and when there is a tweet to display, @@ -63,6 +62,7 @@ class AmpTwitter extends AMP.BaseElement { amp.setAttribute('width', data.width); this./*OK*/changeHeight(data.height); }, /* opt_is3P */true); + this.element.appendChild(iframe); return loadPromise(iframe); } }; From dcb9d9d7be10f0232b481862155571bc0124cf01 Mon Sep 17 00:00:00 2001 From: Justin Ridgewell Date: Tue, 19 Apr 2016 17:14:35 -0700 Subject: [PATCH 4/4] Fix IntersectionObserver tests --- test/functional/test-intersection-observer.js | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/test/functional/test-intersection-observer.js b/test/functional/test-intersection-observer.js index f43bb093dd18..4941fd0125f2 100644 --- a/test/functional/test-intersection-observer.js +++ b/test/functional/test-intersection-observer.js @@ -164,10 +164,13 @@ describe('IntersectionObserver', () => { function getIframe(src) { const i = document.createElement('iframe'); i.src = src; - document.body.appendChild(i); return i; } + function insert(iframe) { + document.body.appendChild(iframe); + } + beforeEach(() => { sandbox = sinon.sandbox.create(); clock = sandbox.useFakeTimers(); @@ -221,8 +224,9 @@ describe('IntersectionObserver', () => { }); it('should not send intersection', () => { - postMessageSpy = sinon/*OK*/.spy(testIframe.contentWindow, 'postMessage'); const ioInstance = new IntersectionObserver(element, testIframe); + insert(testIframe); + postMessageSpy = sinon/*OK*/.spy(testIframe.contentWindow, 'postMessage'); ioInstance.sendElementIntersection_(); expect(postMessageSpy.callCount).to.equal(0); expect(ioInstance.pendingChanges_).to.have.length(0); @@ -230,11 +234,12 @@ describe('IntersectionObserver', () => { it('should send intersection', () => { const messages = []; + const ioInstance = new IntersectionObserver(element, testIframe); + insert(testIframe); sandbox.stub(testIframe.contentWindow, 'postMessage', message => { // Copy because arg is modified in place. messages.push(JSON.parse(JSON.stringify(message))); }); - const ioInstance = new IntersectionObserver(element, testIframe); clock.tick(33); ioInstance.startSendingIntersectionChanges_(); expect(getIntersectionChangeEntrySpy.callCount).to.equal(1); @@ -251,11 +256,12 @@ describe('IntersectionObserver', () => { it('should send more intersections', () => { const messages = []; + const ioInstance = new IntersectionObserver(element, testIframe); + insert(testIframe); sandbox.stub(testIframe.contentWindow, 'postMessage', message => { // Copy because arg is modified in place. messages.push(JSON.parse(JSON.stringify(message))); }); - const ioInstance = new IntersectionObserver(element, testIframe); ioInstance.startSendingIntersectionChanges_(); expect(getIntersectionChangeEntrySpy.callCount).to.equal(1); expect(messages).to.have.length(0); @@ -287,6 +293,7 @@ describe('IntersectionObserver', () => { it('should init listeners when element is in viewport', () => { const fireSpy = sandbox.spy(IntersectionObserver.prototype, 'fire'); const ioInstance = new IntersectionObserver(element, testIframe); + insert(testIframe); ioInstance.onViewportCallback(true); expect(fireSpy.callCount).to.equal(1); expect(onScrollSpy.callCount).to.equal(1); @@ -297,6 +304,7 @@ describe('IntersectionObserver', () => { it('should unlisten listeners when element is out of viewport', () => { const fireSpy = sandbox.spy(IntersectionObserver.prototype, 'fire'); const ioInstance = new IntersectionObserver(element, testIframe); + insert(testIframe); ioInstance.onViewportCallback(true); ioInstance.onViewportCallback(); expect(fireSpy.callCount).to.equal(2); @@ -306,6 +314,7 @@ describe('IntersectionObserver', () => { it('should go into in-viewport state for initially visible element', () => { element.isInViewport = () => true; const ioInstance = new IntersectionObserver(element, testIframe); + insert(testIframe); ioInstance.startSendingIntersectionChanges_(); expect(getIntersectionChangeEntrySpy.callCount).to.equal(2); expect(onScrollSpy.callCount).to.equal(1);