Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

馃毃 Error: Cannot read property '__AMP_TOP' of null #28264

Closed
ampprojectbot opened this issue May 7, 2020 · 9 comments
Closed

馃毃 Error: Cannot read property '__AMP_TOP' of null #28264

ampprojectbot opened this issue May 7, 2020 · 9 comments
Assignees
Labels
Stale Inactive for one year or more Type: Error Report An error reported by AMP Error Reporting WG: analytics

Comments

@ampprojectbot
Copy link
Member

ampprojectbot commented May 7, 2020

Details

Error report: link
First seen: May 4, 2020
Frequency: ~ 1,167/day

Stacktrace

Error: Cannot read property '__AMP_TOP' of null
    at win (src/service.js:325:9)
    at getTopWindow (src/service.js:199:8)
    at getService (src/service.js:378:62)
    at getAmpdoc (src/service.js:251:17)
    at getServiceForDoc (extensions/amp-analytics/0.1/variables.js:449:9)
    at variableServiceForDoc (extensions/amp-analytics/0.1/config.js:62:28)
    at extensions/amp-analytics/0.1/amp-analytics.js:231:32
    at loadConfigTask (extensions/amp-analytics/0.1/amp-analytics.js:237:10)

Notes

@cramforce modified src/service.js:325-325 in #23767 (Aug 7, 2019)
@choumx modified src/service.js:193-202 in #10131 (Jun 28, 2017)
@rsimha modified src/service.js:378-381 in #21212 (May 16, 2019)
@choumx modified src/service.js:246-255 in #16328 (Jun 29, 2018)
@calebcordry modified extensions/amp-analytics/0.1/variables.js:448-449 in #22300 (May 17, 2019)
@zhouyx modified extensions/amp-analytics/0.1/config.js:60-62 in #28174 (May 4, 2020)
@zhouyx modified extensions/amp-analytics/0.1/amp-analytics.js:235-239 in #27605 (Apr 20, 2020)

Seen in:

  • 05-05 Beta (0322)
  • 05-05 Experiment-A (0322)
  • 05-05 Experimental (0322)
  • 05-06 Nightly (2312)
  • +2 more

Possible assignees: @cramforce, @calebcordry

/cc @ampproject/release-on-duty

@ampprojectbot ampprojectbot added the Type: Error Report An error reported by AMP Error Reporting label May 7, 2020
@rcebulko
Copy link
Contributor

rcebulko commented May 7, 2020

Seems to be a variant of #9177 , but this one appeared as soon as the May 5 release hit Nightly and later spiked when that release was promoted to beta/experimental.

@zhouyx It seems likely this is related to #28174 , possibly a race condition.

@rcebulko
Copy link
Contributor

rcebulko commented May 7, 2020

image

@zhouyx
Copy link
Contributor

zhouyx commented May 7, 2020

馃憖

@zhouyx
Copy link
Contributor

zhouyx commented May 7, 2020

I found two related changes.

  1. Expand analytics variables in remote config url聽#28174 as @rcebulko mentioned. The PR added variableServiceForDoc(element).
    Or
  2. Turn on analytics-chunks in canary聽#28069. This PR turned on analytics-chunks in canary. it delays the call of the loadConfig, and thus increase the frequency of the error.

@zhouyx
Copy link
Contributor

zhouyx commented May 7, 2020

After some investigation. I think it's most likely due to #28069.

Reason being we have:

Services.urlReplacementsForDoc(this.element_)

If there's a race condition, the Services.urlReplacementsForDoc(this.element_) should throw the same error before.
I can add additional check to this.win_ != null if that's helpful.

Add @choumx @jridgewell for fixing ideas.

@zhouyx zhouyx self-assigned this May 7, 2020
@dreamofabear
Copy link

Nice catch and thanks for the quick debugging and response. What's the race condition?

@zhouyx
Copy link
Contributor

zhouyx commented May 7, 2020

Read from

amphtml/src/types.js

Lines 85 to 89 in c985e34

/**
* Externs declare that access `defaultView` from `document` or
* `ownerDocument` is of type `(Window|null)` but most of our parameter types
* assume that it is never null. This is OK in practice as we ever only get
* null on disconnected documents or old IE.

My understanding is that element.ownerDocument.defaultView is null and the race condition is that the document is disconnected.

My guess on the error above:

if (isExperimentOn(this.win, 'analytics-chunks') && !this.isInabox_) {
chunk(this.element, loadConfigTask, ChunkPriority.HIGH);
} else {

With the analytics-chunk experiment, the document somehow is disconnected before we call loadConfig?

@dreamofabear
Copy link

Ah ok, it's the same issue. The root fix is probably removing toWin() and unsafe casting.

@stale
Copy link

stale bot commented Nov 29, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

@stale stale bot added the Stale Inactive for one year or more label Nov 29, 2021
@stale stale bot closed this as completed Dec 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Stale Inactive for one year or more Type: Error Report An error reported by AMP Error Reporting WG: analytics
Development

No branches or pull requests

4 participants