js-review

• The SpiderMonkey team works in Stacks of Commits. Each commit should be a logical unit of work. Each unit should be independently reviewable even if the stack must all land as a unit. Patches which mix up work should be split into their constituent parts.
  • Refactoring should be a separate part from functional changes to keep changes clear.
• When fixing a bug, we aim to ensure the bug is fixed everywhere: Are there variants this fix would miss (e.g. with other opcodes), is this a spot fix for a pattern that could be fixed architecturally instead? -- Is there a potential invariant that is being expressed? Is there an assertion we could add that would make catching this easier in the future?
• Prefer MOZ_RELEASE_ASSERT for security critical boundaries; only in very performance-sensitive paths would we relax this.
• Changes should ideally have some description of why they are happening -- either commit message or the bug are acceptable.
• Consider adding a new [SMDOC] comment for large optimizations, subsystems, or data structures. This should not be AI written.

Garbage Collected Data

• GC managed data should be Rooted across a call to a function which can GC. searchfox-cli can identify existing functions which could trigger GC.

Cross-Compartment Wrappers

• Functions should take arguments of all the same compartment as the currently active JSContext (cx->check(...)), or indicate somehow it is possible the arguments do not follow this rule, and if not, we should enter the compartment of an object before manipulating it.

Side Effects

• Many JavaScript operations can have side effects throughout the engine: Ensure state in a frame isn't invalidated by script execution which may not be obvious.

JIT Concerns

• Look very closely at alias-sets and look for paths which can have unexpected side-effects.
• Range analysis is specifically dangerous in cases where it makes claims about ranges, as this feeds into bounds check, and errors here can be exploitable.

OOM and exception handling

• C++ functions that take a JSContext* should typically report an exception on OOM with ReportOutOfMemory(cx).
• Exceptions must be either propagated or cleared, never left on the context.
• Don't just MOZ_CRASH on OOM, use AutoEnterOOMUnsafeRegion -- Prefer this where it is architecturally awkward to communicate the OOM exception state to adding tons of awkward plumbing.

Testing

• Ensure that the tests provide value -- Do not simply test the happy-path, do not add tests where failure is unlikely or impossible.
• For JIT behavior, make sure the test reaches the JIT tier. |jit-test| --fast-warmup can help.