4 ms·

Y'all are very thorough. Just make sure the function has tests and won't blow up prod. You needn't waste forever reviewing. It's code. It can be iterated upon.

londons_explore 3y ago
Indeed. I always push my teams into not hunting for spelling mistakes. Instead look for things that will be hard to fix in the future ('did you realise that this bootloader config stops us doing field updates??') or where the overall direction is wrong ('Not sure about os.system('grep') for this XML - why can't we just use the xml.etree here?').
- js2 3y ago
  > not hunting for spelling mistakes Unless it's the name of an HTTP header?
  - ianlevesque 3y ago
    Not sure what you’re refering to.
    - missblit 3y ago
      Referer https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Referer https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Re...
      - deleted 3y ago
        [deleted]
      - pacaro 3y ago
        Woosh. Also Henrik is Danish, he can claim that English isn't his first language
        js2 3y ago
        Henrik?
        pacaro 3y ago
        I worked for a while with Henrik Frystyk Nielsen, who worked on the early HTTP spec and httpd. We would give him a hard time for this — among other things. I know that Phillip Hallam-Baker probably first used that spelling in a document
      - smcl 3y ago
        You might want to check how that word was spelled in the comment you're replying to :)
    - rezonant 3y ago
      Well done :-D
pdpi 3y ago
In this particular case, we're talking some tricky code around array handling in the JS implementation of a major browser. It's pretty much the archetypical location for a VM escape bug. That's most definitely not something you want to be cavalier about. In general, the large, sprawling diffs we're talking about here, the sort that actually justify commits of this size, are almost always going to also be the sort that justify more closer scrutiny at review time.
- pizlonator 3y ago
  I doubt that a thorough review is going to find security bugs in a change like this. Good tests, a great fuzzer, and a highly motivated security research community - that’s how you find the great security bugs!
xvector 3y ago
This kind of mentality is absurdly inappropriate when working on something as security sensitive as a browser's JS engine.
olliej 3y ago
No. In any actually complex piece of code even adding significant amounts testing over new code is not going to cover every possible code path. It's also immensely difficult to write tests to find errors in code you're writing - most patches I see with "extensive tests" test all the correct code paths, and _maybe_ a few error cases. It's a very easy trap to get yourself into. The purpose of review is not to catch obvious issues - though it hopefully will - but the subtle ones.
- hgsgm 3y ago
  Purpose of review is to ask questions to help you write more tests to catch more bugs.
  - pizlonator 3y ago
    I’ve had both thorough and cursory reviews of changes like this. It makes no difference for finding actual bugs. Reviews are great for spreading knowledge about the change and making sure the style is dialed in. Reviews aren’t great for ensuring test coverage or finding security bugs.
    - eru 3y ago
      Test coverage should be ensured by your coverage tools in the CI. (At least basic test coverage.) You are right that reviews aren't necessarily there to catch bugs. Reviews are there to tell you to write code that's simple and understandable. If your coworkers can't understand what's going on, how could you expect anyone else to do so in a year or so?
      - olliej 3y ago
        Coverage tools only tell you have tests that hit each line, but most serious bugs aren't the "you didn't hit one line", they're the result of specific interactions between multiple paths. e.g. (note this is a super simple and trivial example) int size = 0; char *buffer; if (a) { buffer = new char[1]; size = 1; } else if (b) { buffer = new char[2]; size = 2; } ... if (b) { buffer[1]++; } ... Coverage tests will happily show that every line is hit, without tests necessarily hitting the buffer overflow if an and b are both true.
amne 3y ago
This is code running on client machines which you have no control over. Once it's out there good luck getting everyone to upgrade to your latest version because "hey, we didn't want this one to behave like that. oops". Especially when you're google and don't want certain freedoms in your software.
- 8n4vidtmkvmk 3y ago
  That's fair. I mostly work on servers and web FE which is also easy to redeploy.

londons_explore 3y ago

Indeed. I always push my teams into not hunting for spelling mistakes. Instead look for things that will be hard to fix in the future ('did you realise that this bootloader config stops us doing field updates??') or where the overall direction is wrong ('Not sure about os.system('grep') for this XML - why can't we just use the xml.etree here?').

js2 3y ago
> not hunting for spelling mistakes Unless it's the name of an HTTP header?
- ianlevesque 3y ago
  Not sure what you’re refering to.
  - missblit 3y ago
    Referer https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Referer https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Re...
    - deleted 3y ago
      [deleted]
    - pacaro 3y ago
      Woosh. Also Henrik is Danish, he can claim that English isn't his first language
      - js2 3y ago
        Henrik?
        pacaro 3y ago
        I worked for a while with Henrik Frystyk Nielsen, who worked on the early HTTP spec and httpd. We would give him a hard time for this — among other things. I know that Phillip Hallam-Baker probably first used that spelling in a document
    - smcl 3y ago
      You might want to check how that word was spelled in the comment you're replying to :)
  - rezonant 3y ago
    Well done :-D

pdpi 3y ago

In this particular case, we're talking some tricky code around array handling in the JS implementation of a major browser. It's pretty much the archetypical location for a VM escape bug. That's most definitely not something you want to be cavalier about. In general, the large, sprawling diffs we're talking about here, the sort that actually justify commits of this size, are almost always going to also be the sort that justify more closer scrutiny at review time.

pizlonator 3y ago
I doubt that a thorough review is going to find security bugs in a change like this. Good tests, a great fuzzer, and a highly motivated security research community - that’s how you find the great security bugs!

xvector 3y ago

This kind of mentality is absurdly inappropriate when working on something as security sensitive as a browser's JS engine.

olliej 3y ago

No. In any actually complex piece of code even adding significant amounts testing over new code is not going to cover every possible code path. It's also immensely difficult to write tests to find errors in code you're writing - most patches I see with "extensive tests" test all the correct code paths, and _maybe_ a few error cases. It's a very easy trap to get yourself into. The purpose of review is not to catch obvious issues - though it hopefully will - but the subtle ones.

hgsgm 3y ago
Purpose of review is to ask questions to help you write more tests to catch more bugs.
- pizlonator 3y ago
  I’ve had both thorough and cursory reviews of changes like this. It makes no difference for finding actual bugs. Reviews are great for spreading knowledge about the change and making sure the style is dialed in. Reviews aren’t great for ensuring test coverage or finding security bugs.
  - eru 3y ago
    Test coverage should be ensured by your coverage tools in the CI. (At least basic test coverage.) You are right that reviews aren't necessarily there to catch bugs. Reviews are there to tell you to write code that's simple and understandable. If your coworkers can't understand what's going on, how could you expect anyone else to do so in a year or so?
    - olliej 3y ago
      Coverage tools only tell you have tests that hit each line, but most serious bugs aren't the "you didn't hit one line", they're the result of specific interactions between multiple paths. e.g. (note this is a super simple and trivial example) int size = 0; char *buffer; if (a) { buffer = new char[1]; size = 1; } else if (b) { buffer = new char[2]; size = 2; } ... if (b) { buffer[1]++; } ... Coverage tests will happily show that every line is hit, without tests necessarily hitting the buffer overflow if an and b are both true.

amne 3y ago

This is code running on client machines which you have no control over. Once it's out there good luck getting everyone to upgrade to your latest version because "hey, we didn't want this one to behave like that. oops". Especially when you're google and don't want certain freedoms in your software.

8n4vidtmkvmk 3y ago
That's fair. I mostly work on servers and web FE which is also easy to redeploy.