TypeError: Cannot assign to read only property 'data' of object '#<ImageData>'

[reykjavikingur]

I'm submitting a ... (check one with "x")

[ X ] bug report => search github for a similar issue or PR before submitting
[ ] feature request
[ ] support request => Please do not submit support request here, instead see https://github.com/angular/angular/blob/master/CONTRIBUTING.md#question

Current behavior
In Chrome 57 (not 56 or prior) on Windows or Mac, using Angular 1.5.7 and ui.router module, when calling $state.go and passing state variable object containing instance of ImageData, at some point copyRecurse tries to do a member-wise copy of the ImageData instance, even though it's immutable and should be copied by reference only.

Expected behavior
copyRecurse should detect that the ImageData instance is immutable and not do a member-wise copy on it.

Minimal reproduction of the problem with instructions
Steps:

• Go to https://www.sherwin-williams.com/visualizer#/active/colors
• Select "Match a photo"
• Select an image file in the file system dialog
• Check "I accept Terms of Use"
• Select "Done"
  See the error message in the console.

What is the motivation / use case for changing the behavior?
Whatever reason for recursively copying state variables, stop recursion when you hit an ImageData instance.

Please tell us about your environment:
Windows or Mac. This is happening only on Chrome 57, not any previous versions. We've tested and used this feature for years.

• Angular version: 1.5.7

• Browser: [Chrome 57]

• Language: [ES5]

• Node (for AoT issues): node --version =

[Narretz]

Thanks for the report.
I wonder if that's a Chrome regression? If it worked in C56, maybe they are doing something different / wrong. Firefox doesn't have this problem. :/

Simple repro: https://plnkr.co/edit/7EQVABDTUOd7dzmllXA9

In Firefox, you can see that the resulting copy doesn't throw, but it's actually not usable as a copy. So I assume you don't actually use this copy in your state data?

We could add this case to the copyType method to fix it:

      case '[object ImageData]':
        return new ImageData(new Uint8ClampedArray(source.data), source.width, source.height);

[reykjavikingur]

I'm passing the ImageData instance through $state.go because it's an easy way to communicate it from one state to next, analogous to putting an image encoding in a POST body for HTTP uploads. So, yes, I'm referring to it in the subsequent state. Those ImageData objects can be very large, and I didn't realize at the time that $state would need to make a deep copy of it. What do you think about certain types using pass-by-reference or at least a shallow copy?

[Narretz]

Well, the method is called copy, and we should respect that ;). I'm still wondering why exactly it works like it does - if you run this simple example in FF (https://plnkr.co/edit/veOspfMfsja3OmHu9hjB?p=preview), you can see that the "copied" ImageData is totally clobbered, and totally unusable. I assume in your working example, this is different (because it works in FF). What are you actually passing? A Canvas? Any chance you can break down your example to plnkr size?

[reykjavikingur]

Your plnkr is an accurate representation of the issue. I'm passing an instance of ImageData (not Canvas) to $state.go, which eventually passes it to copyRecurse, as does angular.copy. I also confirmed functional equivalence between your code and my code in Chrome 57 and Firefox 45. In both cases, Chrome 57 throws the error, while Firefox 45 does not throw any error. In fact, in Firefox, the data appears to be fine too.

More to the point, is ImageData a type that angular.copy should be able to handle? I mean, copyElement sets needsRecurse, so in some cases, it returns a reference to the source, rather than trying to copy things like primitives and known immutables. But for some reason, that logic isn't extending to ImageData correctly in Chrome 57.

[Narretz]

Both FF and Chrome get into the "copyRecurse" path - the difference is that Chrome respects the readonly state of the data property and FF doesn't. FF also ends up with a broken object that should be unusable for your purpose, so I'm not sure why your app works.

Btw, have you fixed / introduced a workaround in your app? I can't reproduce the error anymore. Understandable, as it is in running in production.

Can you please reproduce your example in a minimal demo? For the mentioned reasons (FF produces a broken object when copying ImageData directly), I believe your app cannot rely on the copy of the ImageData - it must be something else.

[reykjavikingur]

I created a workaround so that ImageData objects didn't go through the $state calls, which seems more appropriate. I also can't recreate now. I believe the latest build of Chrome v57 has fixed the underlying issue.

[Narretz]

My plnkr still fails in Chrome 58 - as expected, I'd say. If it works for you, then that's good to hear.