Amphtml: <amp-script>: "Illegal mutation" error should provide more information

[morsssss]

Currently, if I try to use setInterval() or setTimeout() on load, or if I try to make mutations >5 seconds after a user gesture, I get an error message like this:

[amp-script] amp-script[script="myscript"].js was terminated due to illegal mutation.

• Most importantly, it would be much more useful to know what the illegal mutation was. Otherwise, as a developer, I don't know what I've done wrong. If I've tried to make changes 5 seconds after a user interaction, or tried a delayed mutation after the hydration phrase on load, I should be told exactly what that is, so I know not to do it again!

• Less importantly, if the error occurs in an inline script, it would be helpful to be told that instead of amp-script[script="myscript"].js.

This probably seems trivial, but letting developers know what they've done wrong will help them find and fix the error quickly!

[morsssss]

@samouri, @choumx, I think this is pretty high-priority. How hard would it be to get more explicit error messages in here?

[choumx]

Thanks for filing this. I think error messages may already be a lot more descriptive after #26401.

You should see console errors like "Dropped 2x CHILD_LIST mutation(s)" before termination. Also, termination only happens for layout=container now.

I've also thought about removing the termination end state entirely since we're now filtering out ("dropping") illegal mutations anyways. However, no-termination can cause the worker and main page to go out of sync which might be more confusing.

[samouri]

However, no-termination can cause the worker and main page to go out of sync which might be more confusing.

Could we "undo" unaccepted mutations in the worker?

[morsssss]

Thanks!

However I'm afraid that "Dropped 2x CHILD_LIST mutation(s)" will confuse external developers completely.

Can you send me a link to the strings we send out? I'd like to suggest friendlier alternatives!

[choumx]

Could we "undo" unaccepted mutations in the worker?

We could, but it's not obvious to me that we'd end up in a better place.

[quickref]

Thanks for filing this. I think error messages may already be a lot more descriptive after #26401.

You should see console errors like "Dropped 2x CHILD_LIST mutation(s)" before termination. Also, termination only happens for layout=container now.

I've also thought about removing the termination end state entirely since we're now filtering out ("dropping") illegal mutations anyways. However, no-termination can cause the worker and main page to go out of sync which might be more confusing.

Hi, we are using amp-script to identify the device ( iOS/Android ) and show the app download image. It worked fine so far, but suddenly we are getting the below exception.
[amp-script] Dropped 2x "ATTRIBUTES" mutation(s); user gesture is required with [layout=container].

Please advise how to debug this issue

[choumx]

@quickref Can you use a layout other than "container"?

[morsssss]

If we can list the mutation types that can be disallowed, and map those to a set of short descriptive phrases, I think it will go a long ways toward solving this.

Looks like mutation types are listed in this TransferrableMutationType enum? And the possible culprits are in mutationTypeToString_ ?

So, ATTRIBUTES, CHARACTER_DATA, CHILD_LIST, PROPERTIES?

[morsssss]

Just popping this up again... I'm happy to create this map of mutation types to descriptive phrases if you can confirm that I've got a complete list. @samouri ?

[samouri]

I'm still learning the basics of worker-dom, but I think mutationTypeToString_ only covers the most common cases.

A full list can be gathered by taking the TransferrrableMutationTypeEnum and only retaining the userVisibleMutationsTypes.

That leaves us with ATTRIBUTES, CHARACTER_DATA, CHILD_LIST, PROPERTIES, OBJECT_MUTATION, OBJECT_CREATION, IMAGE_BITMAP_INSTANCE

[kristoferbaxter]

These expanded errors are a great case for the URL error extraction project, adding them beforehand should be verified for performance impact during boot.

[choumx]

So, ATTRIBUTES, CHARACTER_DATA, CHILD_LIST, PROPERTIES?

I think these are the important ones. The issue is we're just sending over the types rather than details e.g. which element+attribute is affected and what value is illegal? So it'd be a bit more work to plumb those through as well.

[morsssss]

Of course it would be most helpful to do what you say - to let people know what in the DOM they were trying to mutate when we slapped their hand. In the meantime, I can dream up some more human-readable descriptions for these four.

[morsssss]

Actually, trying out a variety of mutations, I was only able to get two of those four error cases. So I need to ask one of you to help me define when these occur!

CHILD_LIST, I get whenever I try to add elements or modify the text of an element. I might assume this applies to any attempt to change the structure of the DOM or an element's content.

CHARACTER_DATA, I might have expected to get when I modified element text with something like textContent, but I didn't. Instead, this failed silently.

ATTRIBUTES, I get whenever I try to change styles. This never triggered PROPERTIES for me.

When I tried to change a literal attribute, like this:

    const main = document.getElementById('main');
    main.setAttribute('title', 'I am the title');

It also failed silently.

So: am I right about ATTRIBUTES and CHILD_LIST?

And are there cases where CHARACTER_DATA or PROPERTIES can get thrown?

Finally, I know it would take some work to provide more information about the attempted mutation. Is it possible at all to write to the console the line number of the code where the mutation was attempted? I imagine it's not, but I thought I'd give it a try :)

[choumx]

Hm, I think the silent failures are due to our sanitizer library. Those are dead simple to add warnings for.

[morsssss]

Thanks!

Should I create a new issue for that?

[morsssss]

Going to make that issue today, I was totally unable to reproduce these silent failures. And I was able to get the CHARACTER_DATA error.

Maybe it was me who was failing, not at all silently...

[morsssss]

Yeah, still can't reproduce.

So, if it's easy to check whether the sanitizer is doing some sanitizing without throwing a notification of some sort, can we do that?

In the meantime, how about something like this:

• CHILD_LIST: Blocked n attempts to modify DOM element children, innerHTML, or the like. For variable-sized <amp-script> containers, a user action has to happen first.
• ATTRIBUTES: Blocked n attempts to modify DOM element attributes, styles, or the like. For variable-sized <amp-script> containers, a user action has to happen first.
• CHARACTER_DATA: Blocked n attempts to modify textContent or the like. For variable-sized <amp-script> containers, a user action has to happen first.

I'm not sure when PROPERTIES gets thrown. Maybe someone could point to me the code? Are we actually throwing ATTRIBUTES sometimes when we should be throwing PROPERTIES?

I know my language there is wordy. I just haven't succeeded in compressing the bit about variable-sized <amp-script> containers. But of course this string could just be stored once...

[choumx]

PROPERTIES is when you modify a property directly e.g. img.src = '...' vs. img.setAttribute('src', '...'). Generally, properties represent DOM state and attributes simply initialize the value of properties. There are many confusing exceptions however.

[morsssss]

Then how about combining ATTRIBUTES and PROPERTIES into a single message:

Blocked _n_ attempts to modify DOM element attributes, properties, or styles. For variable-sized <amp-script> containers, a user action has to happen first.

[morsssss]

I'd be happy to make a PR for this. The code would be simple. I'd just likely need a serious review from the likes of @samouri as I'm not familiar with the code standards in this repo!

[samouri]

@morsssss: I'm always happy to give you a code review! I'm flexible and can do both lighthearted and serious reviews

[sayjeyhi]

I have this error too, any updates?

edit: the error throws when use getBoundingClientRectAsync :

 element.getBoundingClientRectAsync().then(function(data) {
     console.log(data);
 });

[samouri]

@sayjeyhi, this issue is specifically about improving the error messaging around illegal mutations.

Which type of error are you getting? (ATTRIBUTES, OTHER, etc.). I wouldn't expect getBoundingClientRectAsync to cause this. I'll check if I can reproduce it and open a separate issue if so.

[samouri]

@sayjeyhi, I was able to run your example without any errors. Can you provide a fully reproducible case?

[sayjeyhi]

@samouri thank you, I now that this is not a good place to talk about, I miss understood the issue title. actually, I fixed it, it was because of the wrong place to wait for getBoundingClientRectAsync, now https://github.com/ampproject/worker-dom/issues/835 is the problem.

[samouri]

@morsssss, now that the improvements specified in https://github.com/ampproject/amphtml/issues/26511#issuecomment-601937159 are merged, do you think we should close this issue?

I'll open up a separate thread about piping through exactly which elements and attributes are dropped

[morsssss]

Yes! I think #27834 covers this well :)