Friendly Frames Embed Error

[jridgewell]

oa@https://cdn.ampproject.org/rtv/001481049920709/v0/amp-analytics-0.1.js:3:477
mc@https://cdn.ampproject.org/rtv/001481049920709/v0/amp-analytics-0.1.js:112:365
xa@https://cdn.ampproject.org/rtv/001481049920709/v0/amp-analytics-0.1.js:111:110
[native code]
https://cdn.ampproject.org/rtv/001481049920709/v0/amp-analytics-0.1.js:97:249
L@https://cdn.ampproject.org/rtv/001481049920709/v0/amp-analytics-0.1.js:87:430
https://cdn.ampproject.org/rtv/001481049920709/v0/amp-analytics-0.1.js:88:71

Relevant line is https://github.com/ampproject/amphtml/blob/0310284ed51e6e0ad0addd2371a9f3dc491a8204/src/service.js#L338

Is this fixed by https://github.com/ampproject/amphtml/pull/6241?

[dvoytenko]

I'll take a look, but first impression is that it's not related to #6241.

[dvoytenko]

@jridgewell Is there any actual error here? What's the browser?

[jridgewell]

No error message, all coming from iOS Safari 10.1.1.

[jridgewell]

The no error message will likely be fixed by https://github.com/ampproject/amphtml/pull/6428 once we push out canary.

[dvoytenko]

Might be. Is this a new error? How many of these errors are there? From what I can tell, this happens when document.defaultView == null which is sometimes possible when the iframe is removed.

[dvoytenko]

/to @avimehta Is it possible we keep a reference to a friendly iframe elements somewhere in analytics after an embed is removed?

[avimehta]

yes, we do that afaik. Aren't ads iframes removed if they go out of the view and beyond some threshold? visibility code keeps references around afaik.

[jridgewell]

They're very infrequent, and I'm unsure if they're new.

[dvoytenko]

I doubt a4a iframes get removed. But let's confirm if this is the case first. If indeed the case - it's a minor issue since no prod features are affected.

From what I see, the error starts in the instrumentation.js in:

      listenOnceFunc(spec, vars => {
        const el = getElement(this.ampdoc, spec['selector'],
            analyticsElement, spec['selectionMethod']);
        if (el) {
          const attr = getDataParamsFromAttributes(el, undefined,
              VARIABLE_DATA_ATTRIBUTE_KEY);
          for (const key in attr) {
            vars[key] = attr[key];
          }
        }
        callback(new AnalyticsEvent(eventType, vars));
      }, shouldBeVisible, analyticsElement);

And then eventually fails in amp-analytics.js in handleRequestForEvent_:

    return urlReplacementsForDoc(this.element).expandAsync(request)

Whether it's a trigger on "visible" or "hidden" - I can't tell yet.

[dvoytenko]

So, if it's a hidden event - I think an easy way to repro this would be to get a sample document with a A4A embed and send this document unload signal.

[jridgewell]

https://cdn.ampproject.org/c/www.foxnews.com/us/2016/12/08/at-least-40-vehicles-caught-up-in-deadly-michigan-wreck-during-snowstorm.amp.html

[dvoytenko]

Ok. I'm almost certain that this error is benign. @avimehta what's your opinion?

[dvoytenko]

I actually found a way to repro this 100% of the time in the a4a.amp.html sample. Simply calling $0.unlayoutCallback() on the first ad element. In a short order, it fails for the "timer" trigger.

[dvoytenko]

I have partial fix. A fuller fix would likely take some deeper refactoring to disconnect all events from the destroyed analytics tag.