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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Resources: Don't become ready for layout if already scheduled #29389

Merged
merged 3 commits into from
Jul 21, 2020

Conversation

jridgewell
Copy link
Contributor

#29078 is happening way too frequently. Tracking it down, it's caused by elements that use isRelayoutNeeded. This means every time the element is measured (which happens every resource pass for these elements), they're setting their resource state to READY_FOR_LAYOUT. Normally, that's not an issue. But, we also happens to trigger this measurement directly before calling startLayout, which means every single element that uses isRelayoutNeeded would always fire this bug every single time.

This fixes the bug, by only setting to READY_FOR_LAYOUT if the element is in LAYOUT_COMPLETE state (or FAILED). And that prevents the bug because we'd no longer transition away from LAYOUT_SCHEDULED just before calling startLayout.

This re-enables the logging disabled in #29381.

Fixes #29078

@jridgewell jridgewell changed the title Only go to READY_FOR_LAYOUT if already LAYOUT_COMPLETE Only go back to READY_FOR_LAYOUT if already LAYOUT_COMPLETE Jul 20, 2020
@google-cla google-cla bot added the cla: yes label Jul 20, 2020
Copy link

@dreamofabear dreamofabear left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a unit test. :)

Also I think this PR title would be more accurate as "Resource: Don't become ready for layout if already scheduled."

@jridgewell jridgewell changed the title Only go back to READY_FOR_LAYOUT if already LAYOUT_COMPLETE Resources: Don't become ready for layout if already scheduled Jul 21, 2020
@jridgewell jridgewell merged commit 7f59ffb into ampproject:master Jul 21, 2020
@jridgewell jridgewell deleted the startlayout-error-report branch July 21, 2020 02:56
@omerr132
Copy link

Is this fix live? We've seeing this issue again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

🚨 Error: startLayout called but not LAYOUT_SCHEDULED
4 participants