{"action":"created","issue":{"url":"https://api.github.com/repos/ampproject/amphtml/issues/3354","repository_url":"https://api.github.com/repos/ampproject/amphtml","labels_url":"https://api.github.com/repos/ampproject/amphtml/issues/3354/labels{/name}","comments_url":"https://api.github.com/repos/ampproject/amphtml/issues/3354/comments","events_url":"https://api.github.com/repos/ampproject/amphtml/issues/3354/events","html_url":"https://github.com/ampproject/amphtml/issues/3354","id":157114984,"number":3354,"title":"Build Resource Infinite Loop","user":{"login":"jridgewell","id":112982,"avatar_url":"https://avatars.githubusercontent.com/u/112982?v=3","gravatar_id":"","url":"https://api.github.com/users/jridgewell","html_url":"https://github.com/jridgewell","followers_url":"https://api.github.com/users/jridgewell/followers","following_url":"https://api.github.com/users/jridgewell/following{/other_user}","gists_url":"https://api.github.com/users/jridgewell/gists{/gist_id}","starred_url":"https://api.github.com/users/jridgewell/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/jridgewell/subscriptions","organizations_url":"https://api.github.com/users/jridgewell/orgs","repos_url":"https://api.github.com/users/jridgewell/repos","events_url":"https://api.github.com/users/jridgewell/events{/privacy}","received_events_url":"https://api.github.com/users/jridgewell/received_events","type":"User","site_admin":false},"labels":[{"url":"https://api.github.com/repos/ampproject/amphtml/labels/Priority:%20Critical","name":"Priority: Critical","color":"e11d21"}],"state":"open","locked":false,"assignee":{"login":"mkhatib","id":2647,"avatar_url":"https://avatars.githubusercontent.com/u/2647?v=3","gravatar_id":"","url":"https://api.github.com/users/mkhatib","html_url":"https://github.com/mkhatib","followers_url":"https://api.github.com/users/mkhatib/followers","following_url":"https://api.github.com/users/mkhatib/following{/other_user}","gists_url":"https://api.github.com/users/mkhatib/gists{/gist_id}","starred_url":"https://api.github.com/users/mkhatib/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/mkhatib/subscriptions","organizations_url":"https://api.github.com/users/mkhatib/orgs","repos_url":"https://api.github.com/users/mkhatib/repos","events_url":"https://api.github.com/users/mkhatib/events{/privacy}","received_events_url":"https://api.github.com/users/mkhatib/received_events","type":"User","site_admin":false},"milestone":{"url":"https://api.github.com/repos/ampproject/amphtml/milestones/18","html_url":"https://github.com/ampproject/amphtml/milestones/Current","labels_url":"https://api.github.com/repos/ampproject/amphtml/milestones/18/labels","id":1739456,"number":18,"title":"Current","description":"This milestone tracks issues currently being worked on.","creator":{"login":"ericlindley-g","id":16807766,"avatar_url":"https://avatars.githubusercontent.com/u/16807766?v=3","gravatar_id":"","url":"https://api.github.com/users/ericlindley-g","html_url":"https://github.com/ericlindley-g","followers_url":"https://api.github.com/users/ericlindley-g/followers","following_url":"https://api.github.com/users/ericlindley-g/following{/other_user}","gists_url":"https://api.github.com/users/ericlindley-g/gists{/gist_id}","starred_url":"https://api.github.com/users/ericlindley-g/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/ericlindley-g/subscriptions","organizations_url":"https://api.github.com/users/ericlindley-g/orgs","repos_url":"https://api.github.com/users/ericlindley-g/repos","events_url":"https://api.github.com/users/ericlindley-g/events{/privacy}","received_events_url":"https://api.github.com/users/ericlindley-g/received_events","type":"User","site_admin":false},"open_issues":37,"closed_issues":10,"state":"open","created_at":"2016-04-29T23:08:57Z","updated_at":"2016-05-27T16:54:36Z","due_on":null,"closed_at":null},"comments":1,"created_at":"2016-05-27T01:59:54Z","updated_at":"2016-05-27T17:50:13Z","closed_at":null,"body":"We currently have a hidden infinite loop:\r\n\r\nWhen building `<amp-lightbox>`, we [append each child](https://github.com/ampproject/amphtml/blob/master/extensions/amp-lightbox/0.1/amp-lightbox.js#L55-L57) to a new container element. This causes a us to call `attachedCallback` on that child, which [adds](https://github.com/ampproject/amphtml/blob/d90cfc89b48c5aba822f5a2ca099397d7db5fc65/src/custom-element.js#L639) it the list of resources we need to track.\r\n\r\nWhen we add that element to our list of resources, we add it to a list of unbuilt elements (this is our first problem, but we'll come back to it). When we add to the unbuilt elements list, we [try to build](https://github.com/ampproject/amphtml/blob/d90cfc89b48c5aba822f5a2ca099397d7db5fc65/src/service/resources-impl.js#L398) any unbuilt resources.\r\n\r\nAnd here's the infinite loop. Notice that we're _still_ trying to build our `<amp-lightbox>`. Which means the `<amp-lightbox>` is still \"unbuilt\", and it's still in that list of unbuilt elements we're should try to build. So after adding that child element to the list of unbuilts, we'll loop through all of our unbuilts to build them. And lo and behold, one of them is the `<amp-lightbox>`. So we'll build it again, causing us to append all children to a container, causing the `attachedCallback`, causing the child it to be added to our unbuilts again. And again. And again. And again.\r\n\r\nEasy Fix\r\n------------\r\n\r\nSplice the current element before trying to build it. This is not a good solution.\r\n\r\nBetter Fix\r\n-------------\r\n\r\nUse a `currentTryingToBuildOurUnbuilts` mutex. When we enter `#buildReadyResources_`, check if it's true. If it is, return early. If not, set it to true, then do the loop. After the loop, set it to false.\r\n\r\nWhy's this better? We'll avoid a huge call stack. Let it unwind after the `attachedCallback` until we get back to the build loop. Because we're [not caching the unbuilt's length](https://github.com/ampproject/amphtml/blob/d90cfc89b48c5aba822f5a2ca099397d7db5fc65/src/service/resources-impl.js#L410), we'll eventually get to the newly added unbuilt element.\r\n\r\n\r\n- - -\r\n\r\nRemember that first problem I mentioned above? It's that we sometimes wrap our children in a container. But those children may have already been built. But since we fire another `attachedCallback`, we'll add them to the unbuilts again, and try to build again. It's cool to add them to the list again, but we should make sure they're really unbuilt before trying to [`#build`](https://github.com/ampproject/amphtml/blob/d90cfc89b48c5aba822f5a2ca099397d7db5fc65/src/service/resources-impl.js#L1664-L1667) them again."},"comment":{"url":"https://api.github.com/repos/ampproject/amphtml/issues/comments/222211081","html_url":"https://github.com/ampproject/amphtml/issues/3354#issuecomment-222211081","issue_url":"https://api.github.com/repos/ampproject/amphtml/issues/3354","id":222211081,"user":{"login":"dvoytenko","id":726049,"avatar_url":"https://avatars.githubusercontent.com/u/726049?v=3","gravatar_id":"","url":"https://api.github.com/users/dvoytenko","html_url":"https://github.com/dvoytenko","followers_url":"https://api.github.com/users/dvoytenko/followers","following_url":"https://api.github.com/users/dvoytenko/following{/other_user}","gists_url":"https://api.github.com/users/dvoytenko/gists{/gist_id}","starred_url":"https://api.github.com/users/dvoytenko/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/dvoytenko/subscriptions","organizations_url":"https://api.github.com/users/dvoytenko/orgs","repos_url":"https://api.github.com/users/dvoytenko/repos","events_url":"https://api.github.com/users/dvoytenko/events{/privacy}","received_events_url":"https://api.github.com/users/dvoytenko/received_events","type":"User","site_admin":false},"created_at":"2016-05-27T17:50:13Z","updated_at":"2016-05-27T17:50:13Z","body":"@jridgewell @mkhatib Makes sense and let's proceed with the fix. One thing: if the element is already built - we certainly don't need to build it again. But adding it to the \"pending\" set today shouldn't really cause any problems since CustomElement always ensures that it builds only once. Again - it's certainly better not to even try to build it in this case.\r\n\r\nOne case that still would fall through the cracks is rebuild of cloned nodes. I noticed it happening the other day in lightbox as well. This happens because a cloned node will have a new instance of custom element's `ElementProto` and not aware that it's already been built. I filed a new bug for this.\r\n"}}
{"actor":{"display_login":"dvoytenko"}}