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.

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Jump to bottom

Error: Null promise in layoutCallback #21116

Closed
zhouyx opened this issue Feb 26, 2019 · 12 comments
Closed

Error: Null promise in layoutCallback #21116

zhouyx opened this issue Feb 26, 2019 · 12 comments
Assignees
@calebcordry
Labels
Component: A4A fast fetch P3: When Possible Stale Inactive for one year or more Type: Bug Type: Error Report An error reported by AMP Error Reporting WG: monetization

Comments

@zhouyx
Copy link
Contributor

zhouyx commented Feb 26, 2019 •
edited
Loading

Error: Null promise in layoutCallback
at Error (https://raw.githubusercontent.com/ampproject/amphtml/1902191749350/src/log.js:540)
at apply (https://raw.githubusercontent.com/ampproject/amphtml/1902191749350/src/log.js:244)
at apply (https://raw.githubusercontent.com/ampproject/amphtml/1902191749350/src/log.js:257)
at error (https://raw.githubusercontent.com/ampproject/amphtml/1902191749350/extensions/amp-a4a/0.1/amp-a4a.js:987)
at attemptToRenderCreative (https://raw.githubusercontent.com/ampproject/amphtml/1902191749350/extensions/amp-a4a/0.1/amp-a4a.js:972)
at implementation_ (https://raw.githubusercontent.com/ampproject/amphtml/1902191749350/src/custom-element.js:598)
at (https://raw.githubusercontent.com/ampproject/amphtml/1902191749350/src/utils/promise.js:75)
at (https://raw.githubusercontent.com/ampproject/amphtml/1902191749350/src/utils/promise.js:74)
at implementation_ (https://raw.githubusercontent.com/ampproject/amphtml/1902191749350/src/custom-element.js:596)

go/amp-error/CJnxr-aJ1pbyEw
go/amp-error/CNmRyfmivLf3Nw
@keithwrightbos
Copy link
Contributor

Is this still occurring?

@lannka
Copy link
Contributor

lannka commented Dec 3, 2019

as of today, still happing 10K+ a day
https://pantheon.corp.google.com/errors?service=default-cdn-1p&time=P1D&filter=text:Null%20promise%20in%20layoutCallback&order=COUNT_DESC&resolution=OPEN&resolution=ACKNOWLEDGED&project=amp-error-reporting&authuser=0

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

rcebulko commented Mar 19, 2020 •
edited
Loading

What appears to be happening here is that the layoutCallback handler in the Amp4A component tries to call attemptToRenderCreative. However, the adPromise_ (which should resolve with the creative metadata) isn't initialized until initiateAdRequest is called, which happens in either refresh or onLayoutMeasure.

I suspect these are rarely or never executed before the layoutCallback, meaning the initial attempt to render the creative almost always results in this error, and only after a refresh or re-layout occurs is the promise defined and the creative able to render.

If this is indeed the case, I wonder if layoutCallback should try to call refresh or initiateAdRequest before attempting to render the creative. Alternately, adPromise_ could be initially defined as a new Deferred(), so attemptToRenderCreative could still attach a .then handler, and initiateAdRequest could resolve that promise instead of assigning to it.

@keithwrightbos @lannka WDYT?

@zhouyx
Copy link
Contributor Author

zhouyx commented Mar 23, 2020

Add @calebcordry @powerivq

@calebcordry
Copy link
Member

@rcebulko layoutMeasure should happen before layoutCallback.

Started looking into this error, and realized that it is wrapped in a dev().error() My understanding was that we get rid of dev errors in prod, so I am a bit confused why this shows up at all. Maybe I am missing something?

@rcebulko
Copy link
Contributor

My understanding was that we get rid of devAssert assertions & friends in prod, but dev().error() is just how we wrap errors for logging. Maybe @jridgewell can confirm this?

@rcebulko
Copy link
Contributor

@calebcordry I can confirm that dev().error() reports an error, while devAssert gets removed. Is this a user error in configuration, ie. the publisher is trying to use the element the wrong way? If so, we can make this a user().error(). Otherwise, would the suggestion above be a reasonable fix? If you can confirm, I'll throw a PR together with either resolution

Alternately, adPromise_ could be initially defined as a new Deferred(), so attemptToRenderCreative could still attach a .then handler, and initiateAdRequest could resolve that promise instead of assigning to it.

@rcebulko rcebulko mentioned this issue Apr 30, 2020
Closed
@rcebulko
Copy link
Contributor

@calebcordry @powerivq Until recently, this was occurring a bit over 100k times per day. As of the 4/24 release to Beta/Experimental (2004240001480), this error exploded to ~4 million per day, but only on the Experimental channel, and logs suggest predominantly on Android user-agents. See the metrics below:

image
image

This massive spike on 4/24 is believe to be attributed to this error. Note that it does not appear to impact Beta, suggesting this is related to an experiment being run. Note that if this experiment is enabled in production, It would cause on the order of 800 million errors daily.

@rcebulko
Copy link
Contributor

By device:
image

@calebcordry
Copy link
Member

@rcebulko sorry this slipped through the cracks. As far as the spike I think this is real known error fixed by #28070 so this is probably good it showed up in our logs.

Yeah I was wrong about dev().error I am looking more now to determine what is happening to cause this when the bug does not exist.

@rcebulko rcebulko mentioned this issue May 5, 2020
Closed
@calebcordry calebcordry mentioned this issue Jun 11, 2020
Closed
jridgewell added a commit to jridgewell/amphtml that referenced this issue Jun 11, 2020
@jridgewell
Set resource's state to NOT_LAID_OUT when layout is cancelled
d9b4d88 
I think there's a race condition where:

1. The element is schedule for layout
2. The page becomes hidden (swipe to next page, or change tabs)
3. The element's scheduled layout finally starts

I think the `state` is being reset to `NOT_LAID_OUT` or `READY_FOR_LAYOUT`, and somehow the `startLayout` method is being called. Either state is acceptable for `startLayout` for some reason. Really, we should only be accepting the resource when it's `LAYOUT_SCHEDULED`, which happens when the (async) layout flow is started. If anything happens between the start of that flow and the time the `startLayout` method is called, we're in a broken flow.

Re: ampproject#21116
@jridgewell jridgewell mentioned this issue Jun 11, 2020
Merged
jridgewell added a commit to jridgewell/amphtml that referenced this issue Jun 12, 2020
@jridgewell
Prevent layoutCallback if resource state changes after scheduling layout
c825e24 
I think there's a race condition where:

1. The element is schedule for layout
2. The page becomes hidden (swipe to next page, or change tabs)
3. The element's scheduled layout finally starts

I think the `state` is being reset to `NOT_LAID_OUT` or `READY_FOR_LAYOUT`, and somehow the `startLayout` method is being called. Either state is acceptable for `startLayout` for some reason. Really, we should only be accepting the resource when it's `LAYOUT_SCHEDULED`, which happens when the (async) layout flow is started. If anything happens between the start of that flow and the time the `startLayout` method is called, we're in a broken flow.

Re: ampproject#21116
jridgewell added a commit that referenced this issue Jun 24, 2020
@jridgewell
🐛 Prevent layoutCallback if resource state changes after scheduling l…
ff3bac5 
…ayout (#28849)

* Prevent layoutCallback if resource state changes after scheduling layout

I think there's a race condition where:

1. The element is schedule for layout
2. The page becomes hidden (swipe to next page, or change tabs)
3. The element's scheduled layout finally starts

I think the `state` is being reset to `NOT_LAID_OUT` or `READY_FOR_LAYOUT`, and somehow the `startLayout` method is being called. Either state is acceptable for `startLayout` for some reason. Really, we should only be accepting the resource when it's `LAYOUT_SCHEDULED`, which happens when the (async) layout flow is started. If anything happens between the start of that flow and the time the `startLayout` method is called, we're in a broken flow.

Re: #21116

* Fix tests

* Fix inabox resources
@stale
Copy link

stale bot commented Dec 2, 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 Dec 2, 2021
@stale stale bot closed this as completed Dec 11, 2021
@workjmc
Copy link

workjmc commented Nov 28, 2023

i have the issue too on this page after updating wordpress.

may i know how to fix this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@calebcordry calebcordry

Labels
Component: A4A fast fetch P3: When Possible Stale Inactive for one year or more Type: Bug Type: Error Report An error reported by AMP Error Reporting WG: monetization
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
6 participants
@keithwrightbos @zhouyx @rcebulko @lannka @calebcordry @workjmc