Bug 175023

Summary: REGRESSION (r219193): Custom data attribute objects are lost randomly, maybe due to garbage collection
Product: WebKit Reporter: fabian.schwarzkopf
Component: SVGAssignee: Nobody <webkit-unassigned>
Status: RESOLVED DUPLICATE    
Severity: Normal CC: dino, sabouhallawa, schlm3, seb.p.mueller, svillar, webkit-bug-importer, zimmermann
Priority: P2 Keywords: InRadar
Version: Safari Technology Preview   
Hardware: Mac   
OS: Unspecified   
URL: http://live.yworks.com/yfiles-for-html/2.0/view/grapheditor/index.html
See Also: https://bugs.webkit.org/show_bug.cgi?id=172545
Attachments:
Description Flags
Reproduction of lost data attributes on SVG none

Description fabian.schwarzkopf 2017-08-01 08:00:43 PDT
Created attachment 316854 [details]
Reproduction of lost data attributes on SVG

We noticed in the Safari Technology Preview Release 36 (macOS Sierra, 10.12.6) that the demos of our product did not work as expected anymore (e.g. see http://live.yworks.com/yfiles-for-html/2.0/view/grapheditor/index.html).

The transform that is applied to the SVG is often not applied at all or it works for some time but then gets broken randomly (try zooming or select all with CMD+A and then zoom). For optimization reasons, we cache the transformable on the SVG element (as custom attribute) and subsequently only update the cached transformable.

It seems that sometimes this attribute is lost or is not the same anymore as the one that was attached initially. Please find attached to this bug report is a small repro to demonstrate the issue.

Steps to reproduce:
* Open the index.html
* Start zooming in and out with the mousewheel
* At one time the cached instance is not the same anymore and changing its values won't transform the SVG (this is indicated by the green bar turning to red once the issue triggers)

We were able to trigger the problem more often when creating more dummy nodes in the code (see index.html). Supposedly, this triggers the garbage collector which then breaks the cached transformable.

Note: Comparing the repro with the current Chrome implementation shows, that the instance seems to change too, but the transform can still be changed by manipulating the attached object.
Comment 1 Radar WebKit Bug Importer 2017-08-01 17:17:50 PDT
<rdar://problem/33666644>
Comment 2 Alexey Proskuryakov 2017-08-01 17:21:17 PDT
Does this also break the actual product, or just the demos?
Comment 3 fabian.schwarzkopf 2017-08-02 01:07:05 PDT
(In reply to Alexey Proskuryakov from comment #2)
> Does this also break the actual product, or just the demos?

The 'product' is an API / library while the demos are demo implementation of it. Thus you can say, the demos are the product in that sense.

When it fails in the demos, it will fail for our customers in their products, too.
Comment 4 Sergio Villar Senin 2017-08-02 02:13:04 PDT
Would you be able to come up with a reduced test case?
Comment 5 fabian.schwarzkopf 2017-08-02 02:19:45 PDT
The attachment (Reproduction of lost data attributes on SVG) resembles the issue in a reduced test case. Or is an even more reduced test case needed?
Comment 6 Dean Jackson 2017-08-02 06:59:57 PDT
This test case is perfect. I'm close to finding the regression.
Comment 7 Dean Jackson 2017-08-02 08:05:09 PDT
And of course it is already in the title :)
Comment 8 Sergio Villar Senin 2017-08-02 10:12:15 PDT
(In reply to fabian.schwarzkopf from comment #5)
> The attachment (Reproduction of lost data attributes on SVG) resembles the
> issue in a reduced test case. Or is an even more reduced test case needed?

Oh right that's perfectly fine, I had just looked at the link in the first comment.
Comment 9 Alexey Proskuryakov 2017-08-08 22:15:24 PDT
Sergio, do you plan to tackle this regression?
Comment 10 Sergio Villar Senin 2017-08-09 01:15:03 PDT
(In reply to Alexey Proskuryakov from comment #9)
> Sergio, do you plan to tackle this regression?

I have it in my TODO list but I don't have much time ATM. I'm fine with somebody else taking over. Otherwise I'll try to fix it as soon as possible.
Comment 11 Alexey Proskuryakov 2017-08-09 12:40:47 PDT
I think that the new symptom is worse than the one fixed originally, so rolling back seems like the next step then.
Comment 12 Dean Jackson 2017-08-09 12:55:39 PDT
(In reply to Alexey Proskuryakov from comment #11)
> I think that the new symptom is worse than the one fixed originally, so
> rolling back seems like the next step then.

I agree.
Comment 13 Alexey Proskuryakov 2017-08-09 15:08:11 PDT
Rolling back.

*** This bug has been marked as a duplicate of bug 175398 ***
Comment 14 Alexey Proskuryakov 2017-08-09 15:11:19 PDT
> When it fails in the demos, it will fail for our customers in their products, too.

Could you please list your customers' products, or characterize user impact in some other way?
Comment 15 Sebastian Müller 2017-08-10 00:29:42 PDT
I am CTO of yWorks, who produces the API where the bug was found and reported.

Here is an excerpt of our customer list: https://www.yworks.com/company/references

Our product "yFiles for HTML" is affected, which is a library. Also our new end user application (which uses that library) "yEd Live" is affected:

https://www.yworks.com/products/yed-live
https://www.yworks.com/products/yfiles-for-html

The bug makes the product unusable on the preview version of the affected UAs. The applications behave like an image viewer where you disable zooming and panning and interaction becomes impossible. Otherwise this is a very rich graph editing application and with the bug in place it becomes a bad viewer application, only.

Most of the deployments of our products at our customers are in closed intranets or at least inaccessible from the public web, and for many customers we may not disclose usage of our products in their deployments.

About half of the customers for which there are logos here https://www.yworks.com/#references however would be affected by that bug and anyone with an IT background will easily recognize at least 60% of them, I would say.

For example this application here would be affected, too, from what I can tell just "by looking at the JavaScript" of the running app:

https://docs.microsoft.com/en-us/azure/network-watcher/network-watcher-topology-overview

We *could* implement a performance costly workaround, however there's hundreds of deployments on websites that would have to update and probably several thousands of devices which have the software built-in into their management backends which cannot be updated as easily (think complex switches and appliances with built-in web servers and management software).

If you require more details, I can disclose some of them in a private email. Please get in contact with our company via the homepage in that case.

I, too, would agree that a rollback would be the best solution for now.
Comment 16 Sergio Villar Senin 2017-08-29 02:35:14 PDT
This should be fixed in master now. Hopefully you could test it in the next technology preview (not sure about when Safari is going to ship this change).
Comment 17 fabian.schwarzkopf 2017-09-29 08:37:13 PDT
Did the fix not make it into the release of iOS 11.0.1 and its Safari 11? 

I can see the same rendering issues that we have reported here in Safari 11 on our iOS11 ipad.

We also received several bug reports by our customers which have rendering issues on iOS11.
Comment 18 fabian.schwarzkopf 2017-09-29 08:44:19 PDT
Same issue on macOS 10 with Safari 11.

It looks fine in Safari Tech Preview 40, though.
Comment 19 Markus Schlegel 2017-10-02 13:02:38 PDT
We have been hit by this bug last week. Very inconvenient and sad that iOS11 has been released with that known bug.
Comment 20 Sebastian Müller 2017-10-04 03:27:04 PDT
Note: the broken demo in the report has been modified in the meantime so that it does not exhibit the erroneous behavior anymore. The attached testcase is still valid (and broken for current Safari releases) and should be used instead for testing.