Destroy embed before iframe is removed (ampproject#6575)

* Destroy embed before iframe is removed

* cleanup

extensions/amp-a4a/0.1/amp-a4a.js

@@ -152,6 +152,9 @@ export class AmpA4A extends AMP.BaseElement {
  /** @private {?string} */
  this.adUrl_ = null;
 
+ /** @private {?../../../src/friendly-iframe-embed.FriendlyIframeEmbed} */
+ this.friendlyIframeEmbed_ = null;
+
  /** {?AMP.AmpAdUIHandler} */
  this.uiHandler = null;
 
@@ -558,10 +561,18 @@ export class AmpA4A extends AMP.BaseElement {
  unlayoutCallback() {
  this.emitLifecycleEvent('adSlotCleared');
  this.uiHandler.setDisplayState(AdDisplayState.NOT_LAID_OUT);
+
+ // Allow embed to release its resources.
+ if (this.friendlyIframeEmbed_) {
+ this.friendlyIframeEmbed_.destroy();
+ this.friendlyIframeEmbed_ = null;
+ }
+
  // Remove creative and reset to allow for creation of new ad.
  if (!this.layoutMeasureExecuted_) {
  return true;
  }
+
  // TODO(keithwrightbos): is mutate necessary? Could this lead to a race
  // condition where unlayoutCallback fires and during/after subsequent
  // layoutCallback execution, the mutate operation executes causing our
@@ -791,6 +802,7 @@ export class AmpA4A extends AMP.BaseElement {
  installUrlReplacementsForEmbed(this.getAmpDoc(), embedWin,
  new A4AVariableSource(this.getAmpDoc(), embedWin));
  }).then(friendlyIframeEmbed => {
+ this.friendlyIframeEmbed_ = friendlyIframeEmbed;
  // Ensure visibility hidden has been removed (set by boilerplate).
  const frameDoc = friendlyIframeEmbed.iframe.contentDocument ||
  friendlyIframeEmbed.win.document;

extensions/amp-a4a/0.1/test/test-amp-a4a.js

@@ -266,6 +266,7 @@ describe('amp-a4a', () => {
  });
 
  it('for A4A friendly iframe rendering case', () => {
+ expect(a4a.friendlyIframeEmbed_).to.not.exist;
  a4a.onLayoutMeasure();
  return a4a.layoutCallback().then(() => {
  const child = a4aElement.querySelector('iframe[srcdoc]');
@@ -274,6 +275,22 @@ describe('amp-a4a', () => {
  const a4aBody = child.contentDocument.body;
  expect(a4aBody).to.be.ok;
  expect(a4aBody).to.be.visible;
+ expect(a4a.friendlyIframeEmbed_).to.exist;
+ });
+ });
+
+ it('should reset state to null on unlayoutCallback', () => {
+ a4a.buildCallback();
+ a4a.onLayoutMeasure();
+ return a4a.layoutCallback().then(() => {
+ a4a.vsync_.runScheduledTasks_();
+ expect(a4a.friendlyIframeEmbed_).to.exist;
+ const destroySpy = sandbox.spy();
+ a4a.friendlyIframeEmbed_.destroy = destroySpy;
+ a4a.unlayoutCallback();
+ a4a.vsync_.runScheduledTasks_();
+ expect(a4a.friendlyIframeEmbed_).to.not.exist;
+ expect(destroySpy).to.be.calledOnce;
  });
  });
  });

extensions/amp-analytics/0.1/amp-analytics.js

@@ -136,6 +136,12 @@ export class AmpAnalytics extends AMP.BaseElement {
  .then(this.onFetchRemoteConfigSuccess_.bind(this));
  }
 
+ /** @override */
+ detachedCallback() {
+ // TODO(avimehta, #6543): Release all listeners and resources installed by
+ // this element.
+ }
+
  /**
  * Handle successful fetching of (possibly) remote config.
  * @return {!Promise|undefined}
@@ -414,6 +420,14 @@ export class AmpAnalytics extends AMP.BaseElement {
  * @private
  */
  handleRequestForEvent_(request, trigger, event) {
+ // TODO(avimehta, #6543): Remove this code or mark as "error" for the
+ // once destroyed embed release is implemented. See `detachedCallback`.
+ if (!this.element.ownerDocument.defaultView) {
+ const TAG = this.getName_();
+ dev().warn(TAG, 'request against destroyed embed: ', trigger['on']);
+ return Promise.resolve();
+ }
+
  if (!request) {
  const TAG = this.getName_();
  user().error(TAG, 'Ignoring event. Request string ' +

src/service/resource.js

@@ -765,4 +765,13 @@ export class Resource {
  this.pause();
  this.unlayout();
  }
+
+ /**
+ * Disconnect the resource. Mainly intended for embed resources that do not
+ * receive `disconnectedCallback` naturally via CE API.
+ */
+ disconnect() {
+ delete this.element[RESOURCE_PROP_];
+ this.element.disconnectedCallback();
+ }
 }

src/service/resources-impl.js

@@ -448,14 +448,18 @@ export class Resources {
 
  /**
  * @param {!Resource} resource
+ * @param {boolean=} opt_disconnect
  * @private
  */
- removeResource_(resource) {
+ removeResource_(resource, opt_disconnect) {
  const index = this.resources_.indexOf(resource);
  if (index != -1) {
  this.resources_.splice(index, 1);
  }
  resource.pauseOnRemove();
+ if (opt_disconnect) {
+ resource.disconnect();
+ }
  this.cleanupTasks_(resource, /* opt_removePending */ true);
  dev().fine(TAG_, 'element removed:', resource.debugid);
  }
@@ -466,7 +470,7 @@ export class Resources {
  */
  removeForChildWindow(childWin) {
  const toRemove = this.resources_.filter(r => r.hostWin == childWin);
- toRemove.forEach(r => this.removeResource_(r));
+ toRemove.forEach(r => this.removeResource_(r, /* disconnect */ true));
  }
 
  /**

test/functional/test-resource.js

@@ -53,6 +53,7 @@ describe('Resource', () => {
  pauseCallback: () => false,
  resumeCallback: () => false,
  viewportCallback: () => {},
+ disconnectedCallback: () => {},
  togglePlaceholder: () => sandbox.spy(),
  getPriority: () => 2,
  dispatchCustomEvent: () => {},
@@ -724,6 +725,14 @@ describe('Resource', () => {
  expect(resource.isInViewport_).to.equal(false);
  expect(resource.paused_).to.equal(true);
  });
+
+ it('should call disconnectedCallback on remove for built ele', () => {
+ expect(Resource.forElementOptional(resource.element))
+ .to.equal(resource);
+ elementMock.expects('disconnectedCallback').once();
+ resource.disconnect();
+ expect(Resource.forElementOptional(resource.element)).to.not.exist;
+ });
  });
  });
 

test/functional/test-resources.js

@@ -1556,7 +1556,7 @@ describe('Resources mutateElement and collapse', () => {
 });
 
 
-describe('Resources.add', () => {
+describe('Resources.add/remove', () => {
  let sandbox;
  let resources;
  let parent;
@@ -1576,9 +1576,8 @@ describe('Resources.add', () => {
  isUpgraded() {
  return true;
  },
- dispatchCustomEvent() {
- return;
- },
+ pauseCallback() {},
+ dispatchCustomEvent() {},
  };
  element.build = sandbox.spy();
  return element;
@@ -1760,4 +1759,32 @@ describe('Resources.add', () => {
  expect(schedulePassStub).to.not.be.called;
  });
  });
+
+ describe('remove', () => {
+ it('should remove resource and pause', () => {
+ child1.isBuilt = () => true;
+ resources.add(child1);
+ const resource = child1['__AMP__RESOURCE'];
+ const pauseOnRemoveStub = sandbox.stub(resource, 'pauseOnRemove');
+ const disconnectStub = sandbox.stub(resource, 'disconnect');
+ resources.remove(child1);
+ expect(resources.resources_.indexOf(resource)).to.equal(-1);
+ expect(pauseOnRemoveStub).to.be.calledOnce;
+ expect(disconnectStub).to.not.be.called;
+ });
+
+ it('should disconnect resource when embed is destroyed', () => {
+ child1.isBuilt = () => true;
+ resources.add(child1);
+ const resource = child1['__AMP__RESOURCE'];
+ const pauseOnRemoveStub = sandbox.stub(resource, 'pauseOnRemove');
+ const disconnectStub = sandbox.stub(resource, 'disconnect');
+ const childWin = {};
+ resource.hostWin = childWin;
+ resources.removeForChildWindow(childWin);
+ expect(resources.resources_.indexOf(resource)).to.equal(-1);
+ expect(pauseOnRemoveStub).to.be.calledOnce;
+ expect(disconnectStub).to.be.called;
+ });
+ });
 });