http_parser: do not dealloc during kOnExecute

[indutny]

freeParser deallocates Parser instances early if they do not fit
into the free list. This does not play well with recent socket
consumption change, because it will try to deallocate the parser while
executing on its stack.

Fix: #2928

cc @nodejs/collaborators @trevnorris @bnoordhuis

[indutny]

Test is a bit complicated, because the crash happens only on the access to the uninitialized memory.

[indutny]

CI: https://ci.nodejs.org/job/node-test-pull-request/343/

[bnoordhuis]

What commit or commits introduced the regression?

[bnoordhuis]

Consider doing CHECK(p_->refcount_ > 0) first in this function.

[indutny]

The regression happened at: 1bc4468

[indutny]

@bnoordhuis all fixed.

[bnoordhuis]

Test is a bit complicated, because the crash happens only on the access to the uninitialized memory.

Create 1,000 or 10,000 parser objects and close them from within .execute(). One of them is bound to trigger the crash.

The regression happened at: 1bc4468

Can you include that in the commit log? Maybe also reference the first line of its commit log, that saves the log spelunker an extra git show.

[indutny]

@bnoordhuis included. Will work on the test, but it won't be reliable unless started under valgrind.

[indutny]

Thank you!

[bnoordhuis]

By the way, you can initialize it to 1 here.

[trevnorris]

@indutny this is a bit nitty on my part, but mind putting the regression sha in this format: 3b78b85

Find it's easier to parse when looking through history. No worries if you don't though. :)

[trevnorris]

Outside of including a test, don't see anything I would change. LGTM.

[indutny]

Force pushed, PTAL

[indutny]

New CI: https://ci.nodejs.org/job/node-test-pull-request/346/

[thefourtheye]

Wouldn't it be better to have a const here?

[indutny]

I don't think that delete may be called on const pointer... Am I wrong?

[thefourtheye]

@indutny I just tried this

# include <cstdio>

class Test {
  public:
    Test(const char * str) : my_str(str) {
    }

    ~Test() {
      fprintf(stderr, "Cleaning up");
      delete my_str;
    }

  private:
    const char * my_str;
};

int main() {
  Test test(new char[50]);
}

with

➜  Desktop  g++ Test.cpp && ./a.out
Cleaning up
➜  Desktop

[indutny]

-Wall?

[thefourtheye]

@indutny Hmmm, I tried that now. No warnings.

➜  Desktop  g++ -Wall Test.cpp && ./a.out
Cleaning up
➜  Desktop  

[indutny]

Oh, I think you're right.

[indutny]

Anyway, I'm changing the refcount of the parser, so it won't work.

[thefourtheye]

@indutny Ah, right. Sorry for the fuss.

[indutny]

CI is green. Shall I land it?

[trevnorris]

LGTM

[indutny]

Landed in 229a03f, thank you!

[stephank]

Awesome! Thanks guys! 💪

[MylesBorins]

landed in lts-v4.x-staging as bc9f629