check amp-ad iframe_ is null #3670
Conversation
| @@ -394,7 +398,7 @@ class AmpAd extends AMP.BaseElement { | |||
| this.toggleFallback(true); | |||
| } | |||
| // Remove the iframe only if it is not the master. | |||
| if (this.iframe_.name.indexOf('_master') == -1) { | |||
| if (this.iframe_ && this.iframe_.name.indexOf('_master') == -1) { | |||
There was a problem hiding this comment.
What should be the desired state here? If the user navigates back to this tab, they may see the fallback element now whereas usually we would try loading another ad.
There was a problem hiding this comment.
I don't see why checking this.iframe_ is not null here would change behaviors when user navigates back.
Here's what I understand, the only reason this.iframe_ is null here is that it's set in unlayoutCallback() before. And if that's the case we don't need to call removeElement and setthis.iframe_ = null here. Also we are not doing anything when this.iframe_ = null here before, so adding the extra check here shouldn't change anything?
There was a problem hiding this comment.
This just happens if we killed the iframe in the last animation frame. I think this is exactly correct.
One option would be to short circuit the entire callback by if (!this.iframe_) since nothing here makes sense without it.
zhouyx
force-pushed
the
iframe-null-issue
branch
from
7784ce7
to
95792e9
Compare
|
LGTM |
zhouyx
force-pushed
the
iframe-null-issue
branch
from
95792e9
to
4663ba0
Compare
|
Check iframe null and skip |
|
LGTM |
| @@ -379,6 +382,10 @@ class AmpAd extends AMP.BaseElement { | |||
| * @private | |||
| */ | |||
| noContentHandler_() { | |||
| // If iframe is null nothing to do. | |||
There was a problem hiding this comment.
This still leaves us open to the deferred issue. If the ad calls #noContentHandler_ then the ad is unloaded, it will try to access the iframe's name and the iframe is null.
There was a problem hiding this comment.
Oh, right. The same checks needs to be done at the start of the deferMutate callback.
There was a problem hiding this comment.
woops. Thanks @jridgewell for pointing this out! added check in deferMuate
|
LGTM. |
zhouyx
force-pushed
the
iframe-null-issue
branch
from
28b0be8
to
5152755
Compare
#3632