Skip to content
This repository has been archived by the owner on Mar 4, 2020. It is now read-only.

Reduce our patches for node #2

Open
zcbenz opened this issue Dec 12, 2013 · 17 comments
Open

Reduce our patches for node #2

zcbenz opened this issue Dec 12, 2013 · 17 comments
Labels
enhancement

Comments

@zcbenz
Copy link
Contributor

zcbenz commented Dec 12, 2013

Currently we have about 20 commits based on upstream node's head, we should keep our patches as minimum as possible and try to get some of them into upstream.
zcbenz referenced this issue in electron/electron Dec 12, 2013
@zcbenz
Do not depend on node to set include_dirs.
28f5d18 
Part of atom/node#2.
@zcbenz
Copy link
Contributor Author

zcbenz commented Dec 20, 2013

Tried to merge one patch to upstream: nodejs/node-v0.x-archive#6744.

@nathansobo
Copy link

Nice. Glad you're doing this. Looks like they're open to it?

@zcbenz
Copy link
Contributor Author

zcbenz commented Dec 21, 2013

Yeah they are not against it, but it's not easy to persuade them to accept it too.

@probablycorey
Copy link

Any updates on this @zcbenz?

@zcbenz
Copy link
Contributor Author

zcbenz commented Jun 25, 2014

No news yet, but I have added more information in that PR since atom had been publicized, hoping it could add chances to get our patches merged.

@zcbenz
Copy link
Contributor Author

zcbenz commented Aug 15, 2014

A small patch merged to upstream: nodejs/node-v0.x-archive#8122.

@nathansobo
Copy link

Nice work @zcbenz ✨

@zcbenz
Copy link
Contributor Author

zcbenz commented Jan 7, 2015

Another patch merged to io.js.

@zcbenz
Copy link
Contributor Author

zcbenz commented Jan 20, 2015

Another one merged.

@Fishrock123
Copy link
Contributor

@zcbenz is there a way we (io.js / node) can better support you in this? We'd like to help reduce the number floating patches required for embedders, if possible.

@Fishrock123
Copy link
Contributor

It looks like a good amount of the patches are actually on libuv. Have you thought about submitting them to https://github.com/libuv/libuv?

@zcbenz
Copy link
Contributor Author

zcbenz commented Jul 5, 2015

@Fishrock123 For simple patches I usually just create a pull request to the repo, like libuv/libuv#395, but most of our patches are dirty hacks, it takes a lot of efforts to make them decent APIs with test cases, which I don't have time to do now. But eventually when I got time I would love to merge them to upstream.

@Fishrock123
Copy link
Contributor

Right, but don't be afraid to open issues for some things too! We do need to know what is necessary to make embedders' lives better. :)

@ChALkeR ChALkeR mentioned this issue May 30, 2016
Closed
@refack
Copy link
Contributor

refack commented Sep 28, 2017

Hello all,
If you need someone to help port stuff to mainline node, feel free to ping me.

but most of our patches are dirty hacks,

I'm quite sure that beside me there are other node contributors that would be happy to help turn hacks into APIs.

And RE: dc8fe9d there's a PR open to make those opt-in nodejs/node#15454

@zcbenz
Copy link
Contributor Author

zcbenz commented Oct 2, 2017

@refack That would be great help!

@refack
Copy link
Contributor

refack commented Oct 2, 2017

@refack That would be great help!

I'll take a look at the diff. If you could prioritize stuff, that would help me help you ;)

@zcbenz
Copy link
Contributor Author

zcbenz commented Oct 3, 2017

@refack I'm going to go through the patches in this issue.

Hide console window when creating process.
e6afee6

Generally we should have an option in libuv and Node.js for this flag, but it is useless for console apps and in Electron (and other embedders) we definitely want to have it on by default.

So we can probably add a build flag in libuv to turn it on by default.

Make Module.globalPaths a reference.
f4ad6b7

We need this for security, so desktop apps won't accidentally load a module installed on user's system.

Maybe adding adding a new flag like what we did with process._noBrowserGlobals?

Don't set wrapper class in node, otherwise heap snapshot would crash.
6a24dd7

We commented that code out because it conflicts with blink web engine, no idea how to do with it.

Guard against NULL returned by uv_err_name.
124a0f2

Some antivirus and firewall softwares can block socket connections from other software clients, on our side we would get an unnormal errorno which would make uv_err_name return nullptr. I think the biggest challenge is how to represent an unknown error.

codebytere pushed a commit that referenced this issue Dec 12, 2017
@danbev @gibfahn
test: allow tests to pass without internet
568d614 
Currently when running the test without an internet connection there are
two JavaScript test failures and one cctest. The cctest only fails on
Mac as far as I know. (I've only tested using Mac and Linux thus far).

This commit moves the two JavaScript tests to test/internet.

The details for test_inspector_socket_server.cc:

[ RUN      ] InspectorSocketServerTest.FailsToBindToNodejsHost
make[1]: *** [cctest] Segmentation fault: 11
make: *** [test] Error 2

lldb output:

[ RUN      ] InspectorSocketServerTest.FailsToBindToNodejsHost
Process 63058 stopped
* thread #1: tid = 0x7b175, 0x00007fff96d04384
* libsystem_info.dylib`_gai_simple + 87, queue =
* 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1,
* address=0x0)
    frame #0: 0x00007fff96d04384 libsystem_info.dylib`_gai_simple + 87
libsystem_info.dylib`_gai_simple:
->  0x7fff96d04384 <+87>: movw   (%rdx), %ax
    0x7fff96d04387 <+90>: movw   %ax, -0x2a(%rbp)
    0x7fff96d0438b <+94>: movq   %r13, -0x38(%rbp)
    0x7fff96d0438f <+98>: movq   0x18(%rbp), %rcx

(lldb) bt
* thread #1: tid = 0x7b175, 0x00007fff96d04384
* libsystem_info.dylib`_gai_simple + 87, queue =
* 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1,
* address=0x0)
  * frame #0: 0x00007fff96d04384 libsystem_info.dylib`_gai_simple + 87
    frame #1: 0x00007fff96cfe98b libsystem_info.dylib`search_addrinfo +
179
    frame #2: 0x00007fff96cfafef libsystem_info.dylib`si_addrinfo + 2255
    frame #3: 0x00007fff96cfa67b libsystem_info.dylib`getaddrinfo + 179
    frame #4: 0x00000001017d8888
cctest`uv__getaddrinfo_work(w=0x00007fff5fbfe210) + 72 at
getaddrinfo.c:102
    frame #5: 0x00000001017d880e
cctest`uv_getaddrinfo(loop=0x000000010287cb80, req=0x00007fff5fbfe1c8,
cb=0x0000000000000000, hostname="nodejs.org", service="0",
hints=0x00007fff5fbfe268) + 734 at getaddrinfo.c:192
    frame #6: 0x000000010171f781
cctest`node::inspector::InspectorSocketServer::Start(this=0x00007fff5fbfe658)
+ 801 at inspector_socket_server.cc:398
    frame #7: 0x00000001016ed590
cctest`InspectorSocketServerTest_FailsToBindToNodejsHost_Test::TestBody(this=0x0000000105001fd0)
+ 288 at test_inspector_socket_server.cc:593

I'm not sure about the exact cause for this but when using a standalone
c program to simulate this it seems like when the ai_flags
`AI_NUMERICSERV` is set, which is done in inspector_socket_server.cc
line 394, the servname (the port in the FailsToBindToNodejsHost test) is
expected to be a numeric port string to avoid looking it up in
/etc/services. When the port is 0 as is it was before this commit the
segment fault occurs but not if it is non-zero.

PR-URL: nodejs/node#16255
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
codebytere pushed a commit that referenced this issue Mar 1, 2018
@bnoordhuis @MylesBorins
http: remove adapter frame from onParserExecute
9306de2 
Remove a pointless adapter frame  by fixing up the function's formal
parameter count.  Before:

    frame #0: 0x000033257ea446d5 onParserExecute(...)
    frame #1: 0x000033257ea3b93f <adaptor>
    frame #2: 0x000033257ea41959 <internal>
    frame #3: 0x000033257e9840ff <entry>

After:

    frame #0: 0x00000956287446d5 onParserExecute(...)
    frame #1: 0x0000095628741959 <internal>
    frame #2: 0x00000956286840ff <entry>

PR-URL: nodejs/node#17693
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Khaidi Chu <i@2333.moe>
MarshallOfSound pushed a commit that referenced this issue Mar 29, 2018
@bnoordhuis @BridgeAR
http: remove adapter frame from onParserExecute
31addac 
Remove a pointless adapter frame  by fixing up the function's formal
parameter count.  Before:

    frame #0: 0x000033257ea446d5 onParserExecute(...)
    frame #1: 0x000033257ea3b93f <adaptor>
    frame #2: 0x000033257ea41959 <internal>
    frame #3: 0x000033257e9840ff <entry>

After:

    frame #0: 0x00000956287446d5 onParserExecute(...)
    frame #1: 0x0000095628741959 <internal>
    frame #2: 0x00000956286840ff <entry>

PR-URL: nodejs/node#17693
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Khaidi Chu <i@2333.moe>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@probablycorey @nathansobo @refack @zcbenz @Fishrock123