DataCloneError is thrown from history.pushState and console.log after an dispatching a change event within a custom element with a function being passed into it as a property
Reported by
erica.gu...@updater.com,
Jan 4
|
|||||||
Issue descriptionSteps to reproduce the problem: 1. Check out the static reproducer Demo and/or the Source code Demo Site: https://updater.github.io/chromium-ios-bug-reproducer/ Source Code: https://github.com/Updater/chromium-ios-bug-reproducer 2. Follow instructions on static demo 3. Observe errors and issues - and inexplicably fixed by calling gCrWeb.message.getMessageQueue() What is the expected behavior? DataCloneError should not be thrown as these actions all can be taken in regular chrome and other browsers without issues What went wrong? It's unclear what went wrong but it seems to be a perfect storm of many factors creating the problem, including * Using a custom element * Passing in a function over the element boundary * Dispatching a change event on the element * Calling history.pushState or console.log Did this work before? N/A Chrome version: 64.0.3282.112 Channel: n/a OS Version: all Flash Version: This issue duplicates this one i created yesterday -> https://bugs.chromium.org/p/chromium/issues/detail?id=918698&can=2&start=0&num=100&q=&colspec=ID%20Pri%20M%20Stars%20ReleaseBlock%20Component%20Status%20Owner%20Summary%20OS%20Modified&groupby=&sort= Because now i have a Reproducer, I wanted it to get a fresh start. But i cannot seem to locate the button which says i have a test case
,
Jan 4
Thanks for the very detailed feedback! It seems that you are using Chrome 64 which is a quite old version (released in 2017). Try updating (even if you other bug mention Chrome 70). Eugene: Can you take a look/reassign if needed?
,
Jan 4
Thank you for response. I have some findings based on your suggestions 1) Reproducer works (i.e. it doesnt fail as expected) in chrome 70/71 2) Our Application still seems to suffer from critical failures due to the same error message on chrome 70/71 (from our testing with physical devices and reviewing the associated error longs in Sentry from our user base. The error soley occurs in chrome ios but across all versions.) As a result, I am going to hold off on updating the reproducer until I can guarantee I've reproduced the issue as it stands today in the latest version of chrome. On our end, to ensure our users do not suffer critical failures in our application, we may release an update to our app that try/catches around the history.pushState and attempts to "self heal" by calling gCrWeb.message.getMessageQueue on catch. We also will ensure to try/catch around console.logs when applicable. As of now we are still fairly certain we are seeing the same symptoms in the latest version of chrome, but it will require some more investigation to determine what the minimum required code is to reproduce in current chrome. Thanks for your patience and help!
,
Jan 4
,
Jan 4
The issue is confirmed to still be present in chrome 71 on ios, but only in the context of our complex app as of now. I will re-do the work of minimizing the code using the latest version of chrome and update this ticket once I identify the minimum code needed to reproduce on 70+
,
Jan 4
The demo has been updated to "reproduce" within Chrome 71+. I have tested this on the iPad using latest version of chrome as our browserstack instance doesnt seem to support the latest chrome. Please be advised the "simple" version of the reproducer only reproduced in chrome 64 and other older versions of chrome. I have not figured out how to reduce it down beyond the "custom element" reproducer. To reproduce, follow the instructions on the updated Demo. Link is the same. As for the source code, focus on the code within this folder -> https://github.com/Updater/chromium-ios-bug-reproducer/tree/master/reproducer-custom-element . I will give one final try to update the "simple" reproducer to see if i can reduce it down more than the current demo
,
Jan 4
P.S. To get the reproducer to work in chrome 71+, i only had to update the custom-element to make a call to setAttribute('tabindex',0) on the element (i did so in the constructor). Setting tabindex on the simple reproducer did not have the same effect. So know for now tabindex still has a role in this bug
,
Jan 4
OK!! I got my <final> Update for today here. I paired down my larger demo into the simplest possible code that could possibly break (So i hope/think). The link to the simple Demo is as follows: https://updater.github.io/chromium-ios-bug-reproducer/reproducer-simple/dist/ Click the first button, then second button to see error. Click the third button, then second button to see errors get cleared from then on out. Yay! The repository is the same. This is the minimum code (which can also be found here: https://github.com/Updater/chromium-ios-bug-reproducer/tree/master/reproducer-simple) The minimum code is below (minus the gCrWeb button which i added to the simple demo for testing purposes) HTML <chromium-test></chromium-test> <button id="button">Run Test</button> JS document.querySelector('chromium-test').value = { x: () => { } }; class Test extends HTMLElement { constructor() { super(); this.setAttribute('tabindex', 0); } fireChangeEvent() { this.dispatchEvent(new Event('change')); } connectedCallback() { this.innerHTML = ` <button>Reproduce</button> `; this.querySelector('button').addEventListener('click', this.fireChangeEvent) } } customElements.define( 'chromium-test', Test ); const logToConsole = (html) => { const initalHtml = document.querySelector('#console').innerHTML; document.querySelector('#console').innerHTML = html + initalHtml; } document.querySelector('#button').addEventListener('click', () => { let failed = false; try { console.log('a') } catch (ex) { failed = true; logToConsole(`<code><pre>ConsoleLog Exception: ${ex.message}\n${ex.stack}</pre></code>`); } try { history.pushState(null, '', 'b'); } catch (ex) { failed = true; logToConsole(`<code><pre>PushState Exception: ${ex.message}\n${ex.stack}</pre></code>`); } if (!failed) { logToConsole(`<code><pre>Test passed successfully</pre></code>`) } });
,
Jan 8
,
Jan 8
I haven't dug into this *too* deeply, as there are a lot of details here. One issue that may be contributing to this is that ShadowDOM is only partially supported on iOS (and only on iOS 10 and above). https://caniuse.com/#feat=shadowdomv1 Note about partial support from caniuse.com: "Certain CSS selectors do not work (:host > .local-child) and styling slotted content (::slotted) is buggy."
,
Jan 8
kkhorimoto@google.com, please re-look at what i posted in the follow up comments. The issue is reproducible without needing shadowDom. But this issue is reproducible with or without shadowDom The main example explains and shows that there are components with and without shadowDom and it is reproducible on both The simple example (and the code snippet I pasted above) use custom-element, but otherwise use innerHTML and reproduce the issue without shadowDOM. Also, with or without shadowDom, the "partial support" being referred to by canIUse is CSS related. So why one should expect issues with history.pushState and console.log when using shadowDom is unclear. Regardless, since the issue is reproducible without shadowDom it is a moot point. Please let me know how I can further clarify my examples so that further confusion will not occur.
,
Jan 8
Also, please refer to the <simple example> exclusively if you feel that there are too many details - it is very simple and only a few lines of code. 1) Custom element 2) Has a button in the inner html 3) Tabindex is set on the custom element 4) The button when clicked dispatches a change event on the element 5) Later on, calling console.log or history.pushState throws an error 6) Calling __gCrWeb.messages.getMessageQueue() prevents the error from occuring thereafter I will do whatever it takes to clarify this bug as I spent multiple work days creating this reproducer as opposed to just putting in the __gCrWeb fix (that fix is currently live in production for our users since I assume any bug fixes to chromium will not retroactively fix older versions and since this impacts history.pushState, its critical that we had a workaround). I am particularly worried about unintended consequences of using this gCrWeb function, however, from the QA we did the app seems to work seamlessly with it and history.pushState errors are gracefully handled and resolved.
,
Jan 8
oops, let me add important detail 7) to the above list - a function is set as a prop (nested or not nested - makes no difference) on the custom element - this is REQUIRED to reproduce the issue (surprisingly) so it's important to note.
,
Jan 9
I've found the issue and a possible solution. It appears that the message object we create to send data back to the native app via postMessage have functions being appended to them which breaks postMessage by throwing the DataCloneError. We send the message here: https://cs.chromium.org/chromium/src/ios/web/web_state/js/resources/common.js?type=cs&sq=package:chromium&g=0&l=255 and I am able to fix the issue by converting the message to a string then back again by replacing line 255 with this: var strippedMessage = JSON.parse(JSON.stringify(message)); window.webkit.messageHandlers[handlerName].postMessage(strippedMessage); I found this solution here where they had a similar issue occur: https://github.com/azer/prova/issues/12#issuecomment-40967718 erica.gucciardo@, do you know why these functions are being appended and if this is a common practice? The message is created here (https://cs.chromium.org/chromium/src/ios/web/web_state/js/resources/message.js?type=cs&sq=package:chromium&g=0&l=113) and I'd prefer to prevent the modification of the message if possible instead of landing a hack to convert to and then immediately from a string.
,
Jan 9
The functions are being appended because we do not use a framework but use web components and we have a few cases in our complex app where we would like to pass in functions from parents . The fact that this practice works on all the other browsers to me indicates there should be nothing in web standards spec that indicates assigning functions as props on custom elements should not be allowed. This SO seems to indicate that it's suggested to pass in functions to a web component via properties. It even declares this as best practice. I believe we're not the first or last to have this problem but it's obviously difficult to catch in the first place. https://stackoverflow.com/questions/50865614/can-i-pass-function-as-attribute-to-web-component
,
Jan 9
It does seem that we should strip off any functions from the message object in |sendWebKitMessage|. I have a CL to make this change here: crrev.com/c/1403342. I understand that the custom elements are OK here, but I don't understand why using |customElements.define| is modifying the object that is created and passed to |sendWebKitMessage|.
,
Jan 9
,
Jan 9
Thanks Michael for the quick turn around on this. What are your thoughts with regards to making calls to __gCrWeb.message.getMessageQueue(); like so:
```
try {
history.pushState(null, '', state);
} catch (ex) {
try {
// Try to self-heal from a chromium bug
// https://bugs.chromium.org/p/chromium/issues/detail?id=918988
__gCrWeb.message.getMessageQueue();
history.pushState(null, '', state);
} catch (ignore) {}
}
```
2.7% of our user traffic is currently at risk of being unable to use our app; we find this work around resolves the issue. Does it pose significant risk?
,
Jan 9
I recommend against doing that because it will drop all the messages that are queued and probably break internal parts of iOS Chrome. And actually recommend not interacting with __gCrWeb at all because there is no guarantee of any stable API there. However, I do understand the need for a workaround until a fix is released. Could you try to override |__gCrWeb.common.sendWebKitMessage| with the new implementation in crrev.com/c/1403342? Note this is still risky, and will probably break eventually at a point in the future, but it is a better workaround than dropping the items from the queue.
,
Yesterday
(43 hours ago)
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/ed3443dd1e844b13d052630b36c38e45f03eb523 commit ed3443dd1e844b13d052630b36c38e45f03eb523 Author: Olivier Robin <olivierrobin@chromium.org> Date: Mon Jan 21 11:40:16 2019 Limit form handlers to known elements. Some custom elements may not contain the data required by the handlers and are not supported by autofill anyway. Bug: 918988 Change-Id: I2000c76ecea291e51c2345a5d10ef20e173f9d21 Reviewed-on: https://chromium-review.googlesource.com/c/1420698 Commit-Queue: Olivier Robin <olivierrobin@chromium.org> Reviewed-by: Roger McFarlane <rogerm@chromium.org> Reviewed-by: Mike Dougherty <michaeldo@chromium.org> Cr-Commit-Position: refs/heads/master@{#624557} [modify] https://crrev.com/ed3443dd1e844b13d052630b36c38e45f03eb523/components/autofill/ios/form_util/resources/form_handlers.js
,
Today
(12 hours ago)
I just tested this with Olivier's change (crrev.com/c/1420698) and the demo site in #c8 no longer produces an error. This change is in version 73. I will mark this bug fixed, but erica.gucciardo@ please verify that all your cases are fixed with that patch. |
|||||||
►
Sign in to add a comment |
|||||||
Comment 1 by erica.gu...@updater.com
, Jan 4Wow. So as i was thinking about how complicated this was, It occurred to me to reduce it more. So I did. And this is what i have now: Index.html <div id="a"></div> <button id="button">Reproduce</button> <div id="console"></div> JavaScript const logToConsole = (html) => { const initalHtml = document.querySelector('#console').innerHTML; document.querySelector('#console').innerHTML = html + initalHtml; } document.querySelector('#button').addEventListener('click', () => { document.querySelector("#a").value = { x: () => {} }; document.querySelector("#a").dispatchEvent(new Event('change')); try { console.log('a') } catch (ex) { // } try { history.pushState(null, '', 'b'); } catch (ex) { // } }); See screenshot for result Due to this finding I would like to update the reproducer I provided, so I shall do that but a bit later. Hope this does help identify the problem on your side!!191 KB
191 KB View Download