Bugzilla – Full Text Bug Listing
| Summary: | Upgrade NodeGit to 0.19.0 | ||
|---|---|---|---|
| Product: | [ECD] Orion | Reporter: | Remy Suen <remy.suen> |
| Component: | Node | Assignee: | XinYi Jiang <xinyij> |
| Status: | CLOSED FIXED | QA Contact: | |
| Severity: | enhancement | ||
| Priority: | P2 | CC: | kuschel, Michael_Rennie, Silenio_Quarti, snorthov |
| Version: | 13.0 | ||
| Target Milestone: | 16.0 | ||
| Hardware: | All | ||
| OS: | All | ||
| Whiteboard: | |||
| Bug Depends on: | |||
| Bug Blocks: | 493172, 514343 | ||
|
Description
Remy Suen
We are also interested in the following change. We were patching 0.16.0 to include it. Define GIT_SSH_MEMORY_CREDENTIALS for libgit2 PR #949 https://github.com/nodegit/nodegit/pull/949 Tried to update, but it did not go smooth. We crash getting commit logs (i.e RevWalk). It seems like nodegit is double freeing memory. node(80493,0x7fff7204f000) malloc: *** error for object 0x140eb6b10: incorrect checksum for freed object - object was probably modified after being freed. *** set a breakpoint in malloc_error_break to debug Abort trap: 6 (In reply to Silenio Quarti from comment #2) > Tried to update, but it did not go smooth. We crash getting commit logs (i.e > RevWalk). It seems like nodegit is double freeing memory. Silenio, what OS and version of Node are you on? (In reply to Silenio Quarti from comment #2) > Tried to update, but it did not go smooth. We crash getting commit logs (i.e > RevWalk). It seems like nodegit is double freeing memory. Seems okay to me on Windows 10 running Node v5.1.0. Admittedly, that version of Node is actually no longer supported as of NodeGit 0.17.0. I am running Mac with node v4.5.0. I will try upgrading my node version. Node v5.1.0 crashes on Mac as well. You to have incoming changes for the problem to happen. (In reply to Silenio Quarti from comment #6) > You to have incoming changes for the > problem to happen. Thanks for the information, Silenio! The comparing of two branches seems to be the cause. It's fixed in the current master of libgit2 but that's not what NodeGit 0.17.0 is built on. https://github.com/libgit2/libgit2/pull/4105 I have created a failing test against NodeGit that will reproduce the problem. https://github.com/nodegit/nodegit/pull/1220 I have pushed a regression test that will fail on NodeGit 0.17.0 to master. https://git.eclipse.org/c/orion/org.eclipse.orion.client.git/commit/?id=b6ba92f805e8c8b596fb57bd9bebe65701458cbc We might be able to work around the crash by removing this line in commit.js revWalk.sorting(git.Revwalk.SORT.TOPOLOGICAL); I have not been able to crash without it. This actually matches the java implementation better (no sorting is set). Remy, could you confirm it does not crash on Windows as well? (In reply to Silenio Quarti from comment #10) > We might be able to work around the crash by removing this line in commit.js > > revWalk.sorting(git.Revwalk.SORT.TOPOLOGICAL); > > I have not been able to crash without it. This actually matches the java > implementation better (no sorting is set). > > Remy, could you confirm it does not crash on Windows as well? I have confirmed that removing that line will stop the crashing on Windows 10 running the 64-bit Node 6.9.5. Great! If we are going to use 0.17.0, we should upgrade the node and electron versions as well. There's a new pull request open for bumping the libgit2 submodule in NodeGit to the latest from master. https://github.com/nodegit/nodegit/pull/1223 (In reply to Remy Suen from comment #13) > There's a new pull request open for bumping the libgit2 submodule in NodeGit > to the latest from master. > > https://github.com/nodegit/nodegit/pull/1223 NodeGit v0.18.0 has been released. This release includes the aforementioned libgit2 bump which fixes the crash. https://github.com/nodegit/nodegit/releases/tag/v0.18.0 -------------------- v0.17.0 includes libgit2's PR #4049 so any of our existing error message checks will need to be rewritten or they will end up returning `false`. https://github.com/libgit2/libgit2/pull/4049 Here is an example of such problematic code in stash.js on line 103. https://github.com/eclipse/orion.client/blob/00b7fb8a03458792be1dd36e9e99d48f9c419248/modules/orionode/lib/git/stash.js#L103 It may be possible to replace such message error checks with the new API I added in NodeGit PR #1209. https://github.com/nodegit/nodegit/pull/1209 (In reply to Remy Suen from comment #14) > Here is an example of such problematic code in stash.js on line 103. > > https://github.com/eclipse/orion.client/blob/ > 00b7fb8a03458792be1dd36e9e99d48f9c419248/modules/orionode/lib/git/stash. > js#L103 I've added an automated test for this failure case. This will ensure that we update stash.js when we bump the dependency to NodeGit in package.json. https://git.eclipse.org/c/orion/org.eclipse.orion.client.git/commit/?id=8285152b81f8096010c5aa913eb7be73fbeddd15 commit.js seems to be the only other piece of offending Git code where a capitalized character is used for parsing the error message. https://github.com/eclipse/orion.client/blob/00b7fb8a03458792be1dd36e9e99d48f9c419248/modules/orionode/lib/git/commit.js#L653 (In reply to Remy Suen from comment #16) > commit.js seems to be the only other piece of offending Git code where a > capitalized character is used for parsing the error message. > > https://github.com/eclipse/orion.client/blob/ > 00b7fb8a03458792be1dd36e9e99d48f9c419248/modules/orionode/lib/git/commit. > js#L653 Added a test to ensure a smooth upgrade. https://git.eclipse.org/c/orion/org.eclipse.orion.client.git/commit/?id=e6cbab5b437ee7f1aa9e03aac918727e3a90ce90 NodeGit 0.19.0 has been tagged and released. https://github.com/nodegit/nodegit/releases/tag/v0.19.0 (In reply to Michael Rennie from comment #19) > CQ is here: https://dev.eclipse.org/ipzilla/show_bug.cgi?id=13817 After the update, we should also make sure to link to the build binaries on the wiki (like we have been doing): https://wiki.eclipse.org/Orion/Documentation/Developer_Guide/Electron |