🐛 Register `LINKER_CREATED` on ampdoc instead of attribute by zhouyx · Pull Request #28268 · ampproject/amphtml

zhouyx · 2020-05-08T00:43:18Z

TBH I still don't know what's causing the error. But the purpose of the check is to only initiate the linker manager once for each ampdoc. No need to check for ShadowRoot, the attribute can be applied to the document.body.

jridgewell · 2020-05-08T00:56:18Z

ShadowRoot isn't defined in old Chrome, so trying to reference it throws a ReferenceError.

But we shouldn't be using attributes as markers. It seems we have a need to mark arbitrary data on the ampdoc instance (registerTrackingIframe). Maybe we should generalize that into object properties.

zhouyx · 2020-05-08T01:05:13Z

ShadowRoot isn't defined in old Chrome, so trying to reference it throws a ReferenceError.

Thanks! That's good to know. However based on #28220, the error appears very recently despite that the code was merged back in Feb.

But we shouldn't be using attributes as markers. It seems we have a need to mark arbitrary data on the ampdoc instance (registerTrackingIframe). Maybe we should generalize that into object properties.

I like the idea! Will generalize the solution.

jridgewell · 2020-05-08T01:41:47Z

This is an async error being caught by a promise. If there's no catch handler in the promise chain, then usually we'd get a unhandledrejection error event. But, because not everything uses crossorigin=anonymous, unhandledrejection errors get dropped! We'd never receive the error report.

Now, https://github.com/ampproject/amphtml/blame/c985e344b25effca5e8f2a604a3cef11eff6c0d3/extensions/amp-analytics/0.1/amp-analytics.js#L587 breaks the initialization out of the promise chain. It's now happening in chunked event loop. We have wrappers around these event loops that can catch the error and report it properly, even when the error comes from a non-anonymous script.

So, I think the bug was always there, we just never received the reports.

mdmower · 2020-05-08T02:06:38Z

Oh, I remember this (#26683). Sorry, headNode instanceof ShadowRoot isn't the most compatible solution. To give some context, attribute i-amphtml-linker-created is added to <meta name="amp-google-client-id-api" content="googleanalytics"> when LinkerManager inits. Since this meta tag is not scoped to AmpDocShadow instances, I opted to attach the the attribute to <body>.

I like that you plan to abandon use of this attribute as a marker. If you need a quick fix in the meantime...

--- a/extensions/amp-analytics/0.1/linker-manager.js
+++ b/extensions/amp-analytics/0.1/linker-manager.js
@@ -234,7 +234,7 @@ export class LinkerManager {
 
     const headNode = this.ampdoc_.getHeadNode();
     const linkerCreatedEl =
-      headNode instanceof ShadowRoot
+      headNode.toString() === '[object ShadowRoot]'
         ? this.ampdoc_.getBody()
         : headNode.querySelector(
             'meta[name="amp-google-client-id-api"][content="googleanalytics"]'

zhouyx · 2020-05-08T18:56:50Z

Thank you @jridgewell That makes a lot of sense!
I believe the error reported #28264 was surface recently for the same reason.

zhouyx · 2020-05-08T18:59:21Z

Thank all for the suggestion. I've introduced a more generic solution as Justin suggested.

zhouyx · 2020-05-08T19:01:22Z

src/service/ampdoc-impl.js

+   * Attempt to register a service that is allowed/required one for each ampdoc.
+   * Caller need to handle user error when registration returns false.
+   *
+   * Please list your service name below:


I don't want to add extra bytes to the runtime by keeping an enum list. But I do want to keep such a list somewhere for reference so that we don't end up with identical service name. Let me know if there's better way to address : )

Enums get eliminated if unused, and the Enum.FOO reference will be rewritten with whatever the value is if it's used. Enums are basically 0 cost. 😉

That's cool! Switched to enum

jridgewell

Thanks for cleaning this up!

jridgewell · 2020-05-08T19:31:08Z

extensions/amp-analytics/0.1/linker-manager.js

-          );
-
-    if (linkerCreatedEl.hasAttribute(LINKER_CREATED)) {
+    if (!this.ampdoc_.registerService(LINKER_CREATED)) {


Nit: Just return this.ampdoc_.registerService(LINKER_CREATED)?

👍 Good point

jridgewell · 2020-05-08T19:32:40Z

src/service/ampdoc-impl.js

+   * Attempt to register a service that is allowed/required one for each ampdoc.
+   * Caller need to handle user error when registration returns false.
+   *
+   * Please list your service name below:


Enums get eliminated if unused, and the Enum.FOO reference will be rewritten with whatever the value is if it's used. Enums are basically 0 cost. 😉

jridgewell · 2020-05-08T19:34:41Z

src/service/ampdoc-impl.js

-  registerTrackingIframe() {
-    if (this.hasTrackingIframe_) {
-      return false;
+  registerService(name) {


Nit: I wouldn't call this service, to avoid confusion with ampdoc/window services. Maybe "singleton"?

Sounds good. Changed to registerSingleton()

jridgewell · 2020-05-08T21:28:23Z

extensions/amp-iframe/0.1/amp-iframe.js

 * limitations under the License.
 */

+import {AMPDOC_SINGLETON_NAME} from '../../../src/service/ampdoc-impl';


Maybe we should move this elsewhere, with only this in the file? That way there are no transitive imports, and we don't need the presubmit whitelist?

I agreed! Any idea where to move it to. I wish there's a src/const.js file.

That sounds fine to me. Or src/enums.js

mdmower · 2020-05-09T01:04:09Z

src/enums.js

+ * Registred singleton on AMP doc.
+ * @enum {number}
+ */
+export const AMPDOC_SINGLETON_NAME = {


How about just AMPDOC_SINGLETON?

mdmower · 2020-05-09T01:04:51Z

src/enums.js

+ */
+
+/**
+ * Registred singleton on AMP doc.


Registered singleton on AmpDoc.

check document.body

dd5f044

zhouyx requested review from dreamofabear and mdmower May 8, 2020 00:43

googlebot added the cla: yes label May 8, 2020

zhouyx mentioned this pull request May 8, 2020

🚨 Error: ShadowRoot is not defined #28220

Closed

generic object

7c1c0e2

zhouyx commented May 8, 2020

View reviewed changes

zhouyx requested review from jridgewell and removed request for dreamofabear May 8, 2020 19:01

jridgewell reviewed May 8, 2020

View reviewed changes

use enum & rename

82dd6a4

jridgewell approved these changes May 8, 2020

View reviewed changes

dep-check

fb83629

jridgewell reviewed May 8, 2020

View reviewed changes

jridgewell changed the title ~~🐛 Apply LINKER_CREATED attribute to ampdoc.body~~ 🐛 Register LINKER_CREATED on ampdoc instead of attribute May 8, 2020

move enum to its own file

c1ce8a2

zhouyx merged commit c8caf9d into ampproject:master May 9, 2020

zhouyx deleted the linker-shadowroot branch May 9, 2020 01:06

mdmower reviewed May 9, 2020

View reviewed changes

ampprojectbot added the PR use: In Beta / Experimental label May 19, 2020

ampprojectbot added PR use: In Stable and removed PR use: In Stable labels May 27, 2020

Conversation

zhouyx commented May 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jridgewell commented May 8, 2020

Uh oh!

zhouyx commented May 8, 2020

Uh oh!

jridgewell commented May 8, 2020

Uh oh!

mdmower commented May 8, 2020

Uh oh!

zhouyx commented May 8, 2020

Uh oh!

zhouyx commented May 8, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jridgewell left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

zhouyx commented May 8, 2020 •

edited

Loading