search

albrow / vdom

A virtual dom implementation written in go which is compatible with gopherjs
92 stars 12 forks source link

Fix for GopherJS HierarchyRequestError #2

Closed dradtke closed 7 years ago

dradtke commented 7 years ago

This PR fixes two things:

1) Related to https://github.com/albrow/vdom/issues/1, it appears that there's a bug in GopherJS with append() that sometimes causes the slice used as the first parameter to be modified when it's not supposed to be; the HierarchyRequestError was caused by the final index of an element being modified after it had been calculated. My workaround is to replace these uses of append() with a method that explicitly creates a new slice, copies the old one into it, and then sets the final value.

2) Fixing the HierarchyRequestError revealed a bug in the implementation of Remove.Patch(): the act of removing an element from the DOM invalidates the final index value for all sibling nodes following it, so I quickly ran into a slice out-of-bounds error. I fixed this by iterating over the node's remaining siblings and updating each final index value appropriately.

As a side note, I ran into one more issue that occurs when the browser's DOM doesn't exactly match the HTML; for example, a <table> tag missing a <tbody> will have one quietly added, which means that a template like this won't work for complex patching:

<table>
  <tr>...</tr>
</table>

But this one will:

<table>
  <tbody>
    <tr>...</tr>
  </tbody>
</table>

I'm not sure what the fix for that one is, but it's easily avoided by ensuring all your HTML is well-formed and doesn't result in any implicit modifications by the browser.

albrow commented 7 years ago

@dradtke thanks for this fix. I'm in the process of setting up a testing environment again so I can confirm everything still works after these changes.

it appears that there's a bug in GopherJS with append() that sometimes causes the slice used as the first parameter to be modified when it's not supposed to be

Have you opened an issue in GopherJS for this? If true this would be a very big issue. I would like to avoid adding any temporary workarounds if possible.

dradtke commented 7 years ago

I haven't mostly because I don't know how to distill the issue down to something easily reproducible. I'll try to get one opened soon.

dmitshur commented 7 years ago

I'll try to get one opened soon.

Yes, please do! Thanks, it'll be very appreciated!

Edit: FWIW, I've tried some basics like http://www.gopherjs.org/playground/#/B3z9BQtHDu, and couldn't spot any differences in behavior (aside from capacity increase being different, but that doesn't seem to be a violation of Go spec).

dradtke commented 7 years ago

I ended up reproducing the issue in pure Go, ruling out any GopherJS implementation issues. After reading up a little more on the implementation of append(), I'm pretty sure the issue stems from the fact that the backing array is being shared between multiple index slices. append() only returns a value to account for a possible backing array reallocation; it's not actually equivalent to "copy the slice and add these elements to the end of it."

I haven't been able to fully trace the execution to see at which point what gets shared, but the fix will almost certainly be to ensure each new index slice is self-contained and not sharing its backing array with anything else. I'll update the PR with a shorter version than what's currently there, though.

albrow commented 7 years ago

@dradtke thanks for updating this PR. Unfortunately my main development computer has been out of commission for a while and I am unable to test this change. I'll likely be up and running again in the next week or so.

dradtke commented 7 years ago

I just pushed a small change to use a more efficient, straightforward, but 3-line approach to creating each new index array to replace the clever 1-liners. Shouldn't really affect anything other than readability.

albrow commented 7 years ago

@dradtke I was able to get my dev environment up and running again. Unfortunately, the tests are failing on your branch.

The same tests are failing for Chrome, Safari, and Firefox on macOS. Here's the output for Chrome (the output from the other browsers is redundant):

Chrome 56.0.2924 (Mac OS X 10.12.2) Remove works with a single root element FAILED
    Error: runtime error: invalid memory address or nil pointer dereference
        at $callDeferred (js/test.js:1414:17)
        at $panic (js/test.js:1453:3)
        at $throwRuntimeError (js/test.js:2300:4)
        at Object.$throwNilPointerError (js/test.js:29:42)
        at Object.$packages.github.com/albrow/vdom.Element.ptr.Children (js/test.js:31174:11)
        at Object.$packages.github.com/albrow/vdom.Remove.ptr.Patch (js/test.js:31043:15)
        at createAndApplyPatcher (js/test.js:32218:16)
        at testRemoveRootPatcher (js/test.js:32290:8)
        at $b (js/test.js:31750:10)
        at Object.v.$externalizeWrapper (js/test.js:1877:24)
        at ContextKarma.loaded (http://localhost:9876/context.js:151:12)
Chrome 56.0.2924 (Mac OS X 10.12.2) Remove works with a text root FAILED
    Error: runtime error: invalid memory address or nil pointer dereference
        at $callDeferred (js/test.js:1414:17)
        at $panic (js/test.js:1453:3)
        at $throwRuntimeError (js/test.js:2300:4)
        at Object.$throwNilPointerError (js/test.js:29:42)
        at Object.$packages.github.com/albrow/vdom.Element.ptr.Children (js/test.js:31174:11)
        at Object.$packages.github.com/albrow/vdom.Remove.ptr.Patch (js/test.js:31043:15)
        at createAndApplyPatcher (js/test.js:32218:16)
        at testRemoveRootPatcher (js/test.js:32290:8)
        at $b (js/test.js:31758:10)
        at Object.v.$externalizeWrapper (js/test.js:1877:24)
        at ContextKarma.loaded (http://localhost:9876/context.js:151:12)
Chrome 56.0.2924 (Mac OS X 10.12.2) Remove works with a comment root FAILED
    Error: runtime error: invalid memory address or nil pointer dereference
        at $callDeferred (js/test.js:1414:17)
        at $panic (js/test.js:1453:3)
        at $throwRuntimeError (js/test.js:2300:4)
        at Object.$throwNilPointerError (js/test.js:29:42)
        at Object.$packages.github.com/albrow/vdom.Element.ptr.Children (js/test.js:31174:11)
        at Object.$packages.github.com/albrow/vdom.Remove.ptr.Patch (js/test.js:31043:15)
        at createAndApplyPatcher (js/test.js:32218:16)
        at testRemoveRootPatcher (js/test.js:32290:8)
        at $b (js/test.js:31766:10)
        at Object.v.$externalizeWrapper (js/test.js:1877:24)
        at ContextKarma.loaded (http://localhost:9876/context.js:151:12)
Chrome 56.0.2924 (Mac OS X 10.12.2) Diff removes a root element FAILED
    Error: runtime error: invalid memory address or nil pointer dereference
        at $callDeferred (js/test.js:1414:17)
        at $panic (js/test.js:1453:3)
        at $throwRuntimeError (js/test.js:2300:4)
        at Object.$throwNilPointerError (js/test.js:29:42)
        at Object.$packages.github.com/albrow/vdom.Element.ptr.Children (js/test.js:31174:11)
        at Object.$packages.github.com/albrow/vdom.Remove.ptr.Patch (js/test.js:31043:15)
        at typ.$packages.github.com/albrow/vdom.PatchSet.Patch (js/test.js:30959:15)
        at testDiff (js/test.js:32313:18)
        at $b (js/test.js:31873:10)
        at Object.v.$externalizeWrapper (js/test.js:1877:24)
Chrome 56.0.2924 (Mac OS X 10.12.2) Diff removes a root text node FAILED
    Error: runtime error: invalid memory address or nil pointer dereference
        at $callDeferred (js/test.js:1414:17)
        at $panic (js/test.js:1453:3)
        at $throwRuntimeError (js/test.js:2300:4)
        at Object.$throwNilPointerError (js/test.js:29:42)
        at Object.$packages.github.com/albrow/vdom.Element.ptr.Children (js/test.js:31174:11)
        at Object.$packages.github.com/albrow/vdom.Remove.ptr.Patch (js/test.js:31043:15)
        at typ.$packages.github.com/albrow/vdom.PatchSet.Patch (js/test.js:30959:15)
        at testDiff (js/test.js:32313:18)
        at $b (js/test.js:31897:10)
        at Object.v.$externalizeWrapper (js/test.js:1877:24)
Chrome 56.0.2924 (Mac OS X 10.12.2) Diff removes a root comment node FAILED
    Error: runtime error: invalid memory address or nil pointer dereference
        at $callDeferred (js/test.js:1414:17)
        at $panic (js/test.js:1453:3)
        at $throwRuntimeError (js/test.js:2300:4)
        at Object.$throwNilPointerError (js/test.js:29:42)
        at Object.$packages.github.com/albrow/vdom.Element.ptr.Children (js/test.js:31174:11)
        at Object.$packages.github.com/albrow/vdom.Remove.ptr.Patch (js/test.js:31043:15)
        at typ.$packages.github.com/albrow/vdom.PatchSet.Patch (js/test.js:30959:15)
        at testDiff (js/test.js:32313:18)
        at $b (js/test.js:31921:10)
        at Object.v.$externalizeWrapper (js/test.js:1877:24)

I'll a comment about the specific line that I think is causing this error. If at all possible, it would be good for you to be able to run the tests yourself. That way you can have a shorter development cycle (instead of waiting for me to run the tests for you). Let me know if you have any questions or would like help setting up and running the tests (admittedly the process could be improved).

dradtke commented 7 years ago

Thanks for the feedback. I just pushed a fix for what I believe was causing the error, and all the tests are passing on my end.

albrow commented 7 years ago

@dradtke I confirmed the tests pass. Just merged. Thanks again!