From 2aa4a82499d4becd2284cdb482213d541b8804dd Mon Sep 17 00:00:00 2001 From: Daniel Baumann Date: Sun, 28 Apr 2024 16:29:10 +0200 Subject: Adding upstream version 86.0.1. Signed-off-by: Daniel Baumann --- docs/_static/custom_theme.css | 28 + docs/bug-mgmt/guides/bug-types.rst | 29 + docs/bug-mgmt/guides/other-metadata.rst | 28 + docs/bug-mgmt/guides/priority.rst | 27 + docs/bug-mgmt/guides/severity.rst | 85 ++ docs/bug-mgmt/guides/status-flags.rst | 33 + docs/bug-mgmt/index.rst | 40 + docs/bug-mgmt/policies/new-feature-triage.rst | 55 + docs/bug-mgmt/policies/regressions-github.rst | 151 +++ docs/bug-mgmt/policies/triage-bugzilla.rst | 278 +++++ docs/bug-mgmt/processes/accessibility-review.md | 49 + docs/bug-mgmt/processes/doc-requests.rst | 44 + docs/bug-mgmt/processes/fixing-security-bugs.rst | 159 +++ docs/bug-mgmt/processes/labels.rst | 155 +++ docs/bug-mgmt/processes/regressions.rst | 64 ++ docs/bug-mgmt/processes/security-approval.rst | 219 ++++ docs/bug-mgmt/processes/shared-bug-queues.rst | 34 + .../code-quality/coding-style/coding_style_cpp.rst | 867 ++++++++++++++++ .../coding-style/coding_style_general.rst | 18 + .../coding-style/coding_style_java.rst | 68 ++ docs/code-quality/coding-style/coding_style_js.rst | 150 +++ .../coding-style/coding_style_other.rst | 11 + .../coding-style/coding_style_python.rst | 71 ++ .../format_cpp_code_with_clang-format.rst | 272 +++++ docs/code-quality/coding-style/index.rst | 20 + .../coding-style/using_cxx_in_firefox_code.rst | 1065 ++++++++++++++++++++ docs/code-quality/index.rst | 184 ++++ docs/code-quality/lint/create.rst | 329 ++++++ docs/code-quality/lint/index.rst | 38 + docs/code-quality/lint/linters/black.rst | 36 + docs/code-quality/lint/linters/clang-format.rst | 35 + docs/code-quality/lint/linters/clippy.rst | 42 + docs/code-quality/lint/linters/codespell.rst | 36 + .../lint/linters/cpp-virtual-final.rst | 30 + .../lint/linters/eslint-plugin-mozilla.rst | 243 +++++ .../eslint-plugin-mozilla/avoid-Date-timing.rst | 31 + .../linters/eslint-plugin-mozilla/environment.rst | 33 + .../eslint-plugin-mozilla/no-define-cc-etc.rst | 24 + .../eslint-plugin-mozilla/no-throw-cr-literal.rst | 39 + .../no-useless-parameters.rst | 27 + .../use-chromeutils-import.rst | 25 + .../lint/linters/eslint-plugin-spidermonkey-js.rst | 15 + docs/code-quality/lint/linters/eslint.rst | 60 ++ docs/code-quality/lint/linters/file-perm.rst | 42 + docs/code-quality/lint/linters/file-whitespace.rst | 38 + docs/code-quality/lint/linters/flake8.rst | 46 + docs/code-quality/lint/linters/l10n.rst | 45 + docs/code-quality/lint/linters/license.rst | 39 + docs/code-quality/lint/linters/lintpref.rst | 31 + .../lint/linters/mingw-capitalization.rst | 28 + docs/code-quality/lint/linters/perfdocs.rst | 84 ++ docs/code-quality/lint/linters/pylint.rst | 33 + docs/code-quality/lint/linters/rejected-words.rst | 27 + docs/code-quality/lint/linters/rstlinter.rst | 32 + docs/code-quality/lint/linters/rustfmt.rst | 33 + docs/code-quality/lint/usage.rst | 117 +++ docs/code-quality/static-analysis.rst | 251 +++++ docs/conf.py | 127 +++ docs/config.yml | 83 ++ docs/contributing/Code_Review_FAQ.rst | 93 ++ docs/contributing/build/artifact_builds.rst | 174 ++++ .../contributing/build/building_mobile_firefox.rst | 30 + .../committing_rules_and_responsibilities.rst | 198 ++++ docs/contributing/contribution_quickref.rst | 290 ++++++ docs/contributing/debugging/capturing_minidump.rst | 95 ++ .../debugging/debugging_a_hang_on_macos.rst | 10 + .../debugging/debugging_a_minidump.rst | 191 ++++ .../debugging/debugging_firefox_with_gdb.rst | 504 +++++++++ .../debugging/debugging_firefox_with_lldb.rst | 80 ++ .../debugging/debugging_firefox_with_rr.rst | 75 ++ .../debugging/debugging_firefox_with_valgrind.rst | 177 ++++ docs/contributing/debugging/debugging_on_macos.rst | 359 +++++++ .../debugging/debugging_on_windows.rst | 439 ++++++++ docs/contributing/debugging/img/crashlist.jpg | Bin 0 -> 77200 bytes docs/contributing/debugging/img/reporter.jpg | Bin 0 -> 35259 bytes .../debugging/process_dump_task_manager.rst | 69 ++ docs/contributing/debugging/stacktrace_report.rst | 152 +++ docs/contributing/debugging/stacktrace_windbg.rst | 232 +++++ .../debugging/understanding_crash_reports.rst | 328 ++++++ docs/contributing/directory_structure.rst | 575 +++++++++++ docs/contributing/editor.rst | 231 +++++ docs/contributing/how_to_submit_a_patch.rst | 276 +++++ docs/contributing/img/auto_completion.gif | Bin 0 -> 61333 bytes docs/contributing/img/diagnostic_error.gif | Bin 0 -> 201909 bytes docs/contributing/img/find_references.gif | Bin 0 -> 142707 bytes docs/contributing/img/format_selection.gif | Bin 0 -> 206874 bytes docs/contributing/img/goto_definition.gif | Bin 0 -> 249396 bytes docs/contributing/img/rename_symbol.gif | Bin 0 -> 194963 bytes docs/contributing/img/type_hierarchy.gif | Bin 0 -> 138625 bytes docs/contributing/index.rst | 39 + .../contributing/pocket-guide-shipping-firefox.rst | 434 ++++++++ docs/contributing/reviewer_checklist.rst | 181 ++++ docs/contributing/reviews.rst | 104 ++ docs/contributing/stack_quickref.rst | 103 ++ docs/contributing/vcs/mercurial.rst | 208 ++++ docs/contributing/vcs/mercurial_bundles.rst | 61 ++ docs/contributing/vscode.rst | 108 ++ .../crash-reporting/img/default-search-results.png | Bin 0 -> 74498 bytes .../img/default-search-results2.png | Bin 0 -> 97584 bytes docs/crash-reporting/img/facet-search-results.png | Bin 0 -> 19042 bytes docs/crash-reporting/img/facet-search-results2.png | Bin 0 -> 58106 bytes docs/crash-reporting/img/facet-search-results3.png | Bin 0 -> 25830 bytes .../img/narrower-search-results.png | Bin 0 -> 53573 bytes docs/crash-reporting/img/super-search-form.png | Bin 0 -> 26165 bytes docs/crash-reporting/img/super-search-form2.png | Bin 0 -> 28824 bytes docs/crash-reporting/img/super-search-form3.png | Bin 0 -> 40430 bytes docs/crash-reporting/index.rst | 52 + docs/crash-reporting/searching_crash_reports.rst | 257 +++++ docs/crash-reporting/uploading_symbol.rst | 49 + docs/index.rst | 59 ++ docs/jsdoc.json | 5 + docs/setup/building_with_debug_symbols.rst | 61 ++ docs/setup/configuring_build_options.rst | 440 ++++++++ docs/setup/contributing_code.rst | 199 ++++ docs/setup/getting_set_up.rst | 65 ++ docs/setup/index.rst | 21 + docs/setup/linux_32bit_build_on_64bit_OS.rst | 78 ++ docs/setup/linux_build.rst | 282 ++++++ docs/setup/macos_build.rst | 477 +++++++++ docs/setup/windows_build.rst | 476 +++++++++ docs/testing-rust-code/index.md | 123 +++ docs/writing-rust-code/basics.md | 83 ++ docs/writing-rust-code/ffi.md | 240 +++++ docs/writing-rust-code/index.md | 17 + docs/writing-rust-code/xpcom.md | 123 +++ 125 files changed, 15226 insertions(+) create mode 100644 docs/_static/custom_theme.css create mode 100644 docs/bug-mgmt/guides/bug-types.rst create mode 100644 docs/bug-mgmt/guides/other-metadata.rst create mode 100644 docs/bug-mgmt/guides/priority.rst create mode 100644 docs/bug-mgmt/guides/severity.rst create mode 100644 docs/bug-mgmt/guides/status-flags.rst create mode 100644 docs/bug-mgmt/index.rst create mode 100644 docs/bug-mgmt/policies/new-feature-triage.rst create mode 100644 docs/bug-mgmt/policies/regressions-github.rst create mode 100644 docs/bug-mgmt/policies/triage-bugzilla.rst create mode 100644 docs/bug-mgmt/processes/accessibility-review.md create mode 100644 docs/bug-mgmt/processes/doc-requests.rst create mode 100644 docs/bug-mgmt/processes/fixing-security-bugs.rst create mode 100644 docs/bug-mgmt/processes/labels.rst create mode 100644 docs/bug-mgmt/processes/regressions.rst create mode 100644 docs/bug-mgmt/processes/security-approval.rst create mode 100644 docs/bug-mgmt/processes/shared-bug-queues.rst create mode 100644 docs/code-quality/coding-style/coding_style_cpp.rst create mode 100644 docs/code-quality/coding-style/coding_style_general.rst create mode 100644 docs/code-quality/coding-style/coding_style_java.rst create mode 100644 docs/code-quality/coding-style/coding_style_js.rst create mode 100644 docs/code-quality/coding-style/coding_style_other.rst create mode 100644 docs/code-quality/coding-style/coding_style_python.rst create mode 100644 docs/code-quality/coding-style/format_cpp_code_with_clang-format.rst create mode 100644 docs/code-quality/coding-style/index.rst create mode 100644 docs/code-quality/coding-style/using_cxx_in_firefox_code.rst create mode 100644 docs/code-quality/index.rst create mode 100644 docs/code-quality/lint/create.rst create mode 100644 docs/code-quality/lint/index.rst create mode 100644 docs/code-quality/lint/linters/black.rst create mode 100644 docs/code-quality/lint/linters/clang-format.rst create mode 100644 docs/code-quality/lint/linters/clippy.rst create mode 100644 docs/code-quality/lint/linters/codespell.rst create mode 100644 docs/code-quality/lint/linters/cpp-virtual-final.rst create mode 100644 docs/code-quality/lint/linters/eslint-plugin-mozilla.rst create mode 100644 docs/code-quality/lint/linters/eslint-plugin-mozilla/avoid-Date-timing.rst create mode 100644 docs/code-quality/lint/linters/eslint-plugin-mozilla/environment.rst create mode 100644 docs/code-quality/lint/linters/eslint-plugin-mozilla/no-define-cc-etc.rst create mode 100644 docs/code-quality/lint/linters/eslint-plugin-mozilla/no-throw-cr-literal.rst create mode 100644 docs/code-quality/lint/linters/eslint-plugin-mozilla/no-useless-parameters.rst create mode 100644 docs/code-quality/lint/linters/eslint-plugin-mozilla/use-chromeutils-import.rst create mode 100644 docs/code-quality/lint/linters/eslint-plugin-spidermonkey-js.rst create mode 100644 docs/code-quality/lint/linters/eslint.rst create mode 100644 docs/code-quality/lint/linters/file-perm.rst create mode 100644 docs/code-quality/lint/linters/file-whitespace.rst create mode 100644 docs/code-quality/lint/linters/flake8.rst create mode 100644 docs/code-quality/lint/linters/l10n.rst create mode 100644 docs/code-quality/lint/linters/license.rst create mode 100644 docs/code-quality/lint/linters/lintpref.rst create mode 100644 docs/code-quality/lint/linters/mingw-capitalization.rst create mode 100644 docs/code-quality/lint/linters/perfdocs.rst create mode 100644 docs/code-quality/lint/linters/pylint.rst create mode 100644 docs/code-quality/lint/linters/rejected-words.rst create mode 100644 docs/code-quality/lint/linters/rstlinter.rst create mode 100644 docs/code-quality/lint/linters/rustfmt.rst create mode 100644 docs/code-quality/lint/usage.rst create mode 100644 docs/code-quality/static-analysis.rst create mode 100644 docs/conf.py create mode 100644 docs/config.yml create mode 100644 docs/contributing/Code_Review_FAQ.rst create mode 100644 docs/contributing/build/artifact_builds.rst create mode 100644 docs/contributing/build/building_mobile_firefox.rst create mode 100644 docs/contributing/committing_rules_and_responsibilities.rst create mode 100644 docs/contributing/contribution_quickref.rst create mode 100644 docs/contributing/debugging/capturing_minidump.rst create mode 100644 docs/contributing/debugging/debugging_a_hang_on_macos.rst create mode 100644 docs/contributing/debugging/debugging_a_minidump.rst create mode 100644 docs/contributing/debugging/debugging_firefox_with_gdb.rst create mode 100644 docs/contributing/debugging/debugging_firefox_with_lldb.rst create mode 100644 docs/contributing/debugging/debugging_firefox_with_rr.rst create mode 100644 docs/contributing/debugging/debugging_firefox_with_valgrind.rst create mode 100644 docs/contributing/debugging/debugging_on_macos.rst create mode 100644 docs/contributing/debugging/debugging_on_windows.rst create mode 100644 docs/contributing/debugging/img/crashlist.jpg create mode 100644 docs/contributing/debugging/img/reporter.jpg create mode 100644 docs/contributing/debugging/process_dump_task_manager.rst create mode 100644 docs/contributing/debugging/stacktrace_report.rst create mode 100644 docs/contributing/debugging/stacktrace_windbg.rst create mode 100644 docs/contributing/debugging/understanding_crash_reports.rst create mode 100644 docs/contributing/directory_structure.rst create mode 100644 docs/contributing/editor.rst create mode 100644 docs/contributing/how_to_submit_a_patch.rst create mode 100644 docs/contributing/img/auto_completion.gif create mode 100644 docs/contributing/img/diagnostic_error.gif create mode 100644 docs/contributing/img/find_references.gif create mode 100644 docs/contributing/img/format_selection.gif create mode 100644 docs/contributing/img/goto_definition.gif create mode 100644 docs/contributing/img/rename_symbol.gif create mode 100644 docs/contributing/img/type_hierarchy.gif create mode 100644 docs/contributing/index.rst create mode 100644 docs/contributing/pocket-guide-shipping-firefox.rst create mode 100644 docs/contributing/reviewer_checklist.rst create mode 100644 docs/contributing/reviews.rst create mode 100644 docs/contributing/stack_quickref.rst create mode 100644 docs/contributing/vcs/mercurial.rst create mode 100644 docs/contributing/vcs/mercurial_bundles.rst create mode 100644 docs/contributing/vscode.rst create mode 100644 docs/crash-reporting/img/default-search-results.png create mode 100644 docs/crash-reporting/img/default-search-results2.png create mode 100644 docs/crash-reporting/img/facet-search-results.png create mode 100644 docs/crash-reporting/img/facet-search-results2.png create mode 100644 docs/crash-reporting/img/facet-search-results3.png create mode 100644 docs/crash-reporting/img/narrower-search-results.png create mode 100644 docs/crash-reporting/img/super-search-form.png create mode 100644 docs/crash-reporting/img/super-search-form2.png create mode 100644 docs/crash-reporting/img/super-search-form3.png create mode 100644 docs/crash-reporting/index.rst create mode 100644 docs/crash-reporting/searching_crash_reports.rst create mode 100644 docs/crash-reporting/uploading_symbol.rst create mode 100644 docs/index.rst create mode 100644 docs/jsdoc.json create mode 100644 docs/setup/building_with_debug_symbols.rst create mode 100644 docs/setup/configuring_build_options.rst create mode 100644 docs/setup/contributing_code.rst create mode 100644 docs/setup/getting_set_up.rst create mode 100644 docs/setup/index.rst create mode 100644 docs/setup/linux_32bit_build_on_64bit_OS.rst create mode 100644 docs/setup/linux_build.rst create mode 100644 docs/setup/macos_build.rst create mode 100644 docs/setup/windows_build.rst create mode 100644 docs/testing-rust-code/index.md create mode 100644 docs/writing-rust-code/basics.md create mode 100644 docs/writing-rust-code/ffi.md create mode 100644 docs/writing-rust-code/index.md create mode 100644 docs/writing-rust-code/xpcom.md (limited to 'docs') diff --git a/docs/_static/custom_theme.css b/docs/_static/custom_theme.css new file mode 100644 index 0000000000..4f76e575f4 --- /dev/null +++ b/docs/_static/custom_theme.css @@ -0,0 +1,28 @@ +/* This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ + +/* Increase the size of the content */ +.wy-nav-content { + max-width: 80% !important; +} + +/* Increase the size of the tables */ +table.docutils { + width: 90%; +} + +/* Override the default values for multiline text in a table */ +table.docutils td, table.docutils th +{ + font-size: 16px !important; +} + +.rst-content .line-block { + margin-bottom: 0px !important; +} + +/* Add the strikethrough feature */ +span.strikethrough { + text-decoration: line-through; +} diff --git a/docs/bug-mgmt/guides/bug-types.rst b/docs/bug-mgmt/guides/bug-types.rst new file mode 100644 index 0000000000..3626336de1 --- /dev/null +++ b/docs/bug-mgmt/guides/bug-types.rst @@ -0,0 +1,29 @@ +Bug Types +========= + +We organize bugs by type to make it easier to make triage decisions, get +the bug to the right person to make a decision, and understand release +quality. + +- **Defect** regression, crash, hang, security vulnerability and any + other reported issue +- **Enhancement** new feature, improvement in UI, performance, etc. and + any other request for user-facing enhancements to the product, not + engineering changes +- **Task** refactoring, removal, replacement, enabling or disabling of + functionality and any other engineering task + +All bug types need triage decisions. Engineering :ref:`triages defects and +tasks `. Product management :ref:`triages +enhancements `. + +It’s important to distinguish an enhancement from other types because +they use different triage queues. + +Distinguishing between defects and tasks is important because we want to +understand code quality and reduce the number of defects we introduce as +we work on new features and fix existing defects. + +When triaging, a task can be as important as a defect. A behind the +scenes change to how a thread is handled can affect performance as seen +by a user. diff --git a/docs/bug-mgmt/guides/other-metadata.rst b/docs/bug-mgmt/guides/other-metadata.rst new file mode 100644 index 0000000000..f1b94f16d8 --- /dev/null +++ b/docs/bug-mgmt/guides/other-metadata.rst @@ -0,0 +1,28 @@ +Other Bug Metadata +================== + +Performance +----------- + +- Use the ``perf`` keyword +- Add ``[qf:?]`` to the whiteboard if you think the Performance team + should look at this bug + +Privacy +------- + +- Use the ``privacy`` keyword + +User Security +------------- + +- Will this bug adversely affect Firefox users if left public? + + - Add to security group + +- Otherwise move bug to one of: + + - Core:: Security + - Firefox:: Security + - Toolkit:: Security + - Webkit:: Security diff --git a/docs/bug-mgmt/guides/priority.rst b/docs/bug-mgmt/guides/priority.rst new file mode 100644 index 0000000000..db0c8ee874 --- /dev/null +++ b/docs/bug-mgmt/guides/priority.rst @@ -0,0 +1,27 @@ +Priority Definitions +==================== + +We use these definitions across all components: + ++----------------------------------------+-----------------------------+ +| Priority | Description | ++========================================+=============================+ +| \- | No decision | ++----------------------------------------+-----------------------------+ +| P1 | Fix in the current release | +| | cycle | ++----------------------------------------+-----------------------------+ +| P2 | Fix in the next release | +| | cycle or the following | +| | (nightly + 1 or nightly + | +| | 2) | ++----------------------------------------+-----------------------------+ +| P3 | Backlog | ++----------------------------------------+-----------------------------+ +| P4 | Do not use, this priority | +| | is for web platform test | +| | bots | ++----------------------------------------+-----------------------------+ +| P5 | Will not fix, but will | +| | accept a patch | ++----------------------------------------+-----------------------------+ diff --git a/docs/bug-mgmt/guides/severity.rst b/docs/bug-mgmt/guides/severity.rst new file mode 100644 index 0000000000..c292da19cc --- /dev/null +++ b/docs/bug-mgmt/guides/severity.rst @@ -0,0 +1,85 @@ +Defect Severity +=============== + +Definition +---------- + +We use the ``severity`` field in Bugzilla to indicate the scope of a +bug's effect on Firefox. + +The field is display alongside the bug's priority. + +Values +------ + +`Severities are +enumerated `__ +as: + +- ``--``: Default for new bugs +- ``N/A``: (not applicable): Only applies to bugs of type Task or Enhancement. +- ``S1``: (Catastrophic) Blocks development/testing, may impact more than 25% + of users, causes data loss, potential chemspill, and no workaround available +- ``S2``: (Serious) Major Functionality/product severely impaired and a + satisfactory workaround doesn't exist +- ``S3``: (Normal) Blocks non-critical functionality and a work around exists +- ``S4``: (Small/Trivial) minor significance, cosmetic issues, low or no impact to users + +By default, new bugs have a severity of ``--``. + +Examples of S1 bugs +^^^^^^^^^^^^^^^^^^^ + +- WebExtensions disabled for all users +- Web search not working from URL bar +- Crashes with data loss + +Examples of S2 bugs +^^^^^^^^^^^^^^^^^^^ + +Bugs that could reasonably be expected to cause a Firefox user to switch browsers, +either because the severity is bad enough, or the frequency of occurrence is high enough. + +- `Bug 1640441 `__ - Slack hangs + indefinitely in a onResize loop +- `Bug 1645651 `__ - Changes in + Reddit's comment section JS code makes selecting text slow on Nightly + +Bugs involving contractual partners (if not an S1) + +Bugs reported from QA + +- `Bug 1640913 `__ - because an + important message is not visible with the dark theme. It's not marked as S1 since the + issue is reproducible only on one OS and the functionality is not affected. +- `Bug 1641521 `__ - because videos + are not working on several sites with ETP on (default). This is not an S1 since turning + ETP off fixes the issue. + +Examples of S3 bugs +^^^^^^^^^^^^^^^^^^^ + +Bugs filed by contributors as part of daily refactoring and maintenance of the code base. + +`Bug 1634171 `__ - Visual artifacts around circular images + +Bugs reported from QA + +- `Bug 1635105 `__ because + the associated steps to reproduce are uncommon, + and the issue is no longer reproducible after refresh. +- `Bug 1636063 `__ since it's + reproducible only on a specific web app, and only with a particular set of configurations. + + +Rules of thumb +-------------- + +- *A crash may be be a ``S1`` or ``S2`` defect, but not all crashes are + critical defects* +- High severity defects (``S1`` or ``S2``) do not need to be assigned + immediately as they will be reviewed by Engineering Leadership, QA, and + Release Management +- The severity of most bugs of type ``task`` and ``enhancement`` will be + ``N/A`` +- **Do not** assign bugs of type ``defect`` the severity ``N/A`` diff --git a/docs/bug-mgmt/guides/status-flags.rst b/docs/bug-mgmt/guides/status-flags.rst new file mode 100644 index 0000000000..d3a67f993b --- /dev/null +++ b/docs/bug-mgmt/guides/status-flags.rst @@ -0,0 +1,33 @@ +Release Status Flags +==================== + +The flag ``status_firefoxNN`` has many values, here’s a cheat sheet. + +== ========== ========== ============ ================= +— ? unaffected affected fixed +== ========== ========== ============ ================= +? unaffected wontfix verified +\ affected fix-optional disabled +\ fixed verified-disabled +== ========== ========== ============ ================= + +The headers of the table are values of the status flag. Each column are +the states reachable from the column headings. + +- ``---`` we don’t know whether Firefox N is affected +- ``?`` we don’t know whether Firefox N is affected, but we want to find + out. +- ``affected`` - present in this release +- ``unaffected`` - not present in this release +- ``fixed`` - a contributor has landed a change set in the tree + to address the issue +- ``verified`` - the fix has been verified by QA or other contributors +- ``disabled`` - the fix or the feature has been backed out or disabled +- ``verified disabled`` - QA or other contributors confirmed the fix or + the feature has been backed out or disabled +- ``wontfix`` - we have decided not to accept/uplift a fix for this + release cycle (it is not the same as the bug resolution WONTFIX). + This can also mean that we don’t know how to fix that and will ship + with this bug +- ``fix-optional`` - we would take a fix for the current release but + don’t consider it as important/blocking for the release diff --git a/docs/bug-mgmt/index.rst b/docs/bug-mgmt/index.rst new file mode 100644 index 0000000000..215b2e7317 --- /dev/null +++ b/docs/bug-mgmt/index.rst @@ -0,0 +1,40 @@ +Bug Handling +============ + +Guides +------ + +.. toctree:: + :maxdepth: 1 + :glob: + + guides/* + +Policies +-------- + +.. toctree:: + :maxdepth: 1 + :glob: + + policies/* + +Processes +--------- + +.. toctree:: + :maxdepth: 1 + :glob: + + processes/* + +Related documentation +--------------------- + +- `bugzilla.mozilla.org documentation `__ +- `bugzilla.mozilla.org field + definitions `__ +- `Lando + documentation `__ +- `Mozilla Phabricator (Code Review) + documentation `__ diff --git a/docs/bug-mgmt/policies/new-feature-triage.rst b/docs/bug-mgmt/policies/new-feature-triage.rst new file mode 100644 index 0000000000..7bc8022777 --- /dev/null +++ b/docs/bug-mgmt/policies/new-feature-triage.rst @@ -0,0 +1,55 @@ +New Feature Triage +================== + +Identifying Feature Requests +---------------------------- + +Bugs which request new features or enhancements should be of +type=\ ``enhancement``. + +Older bugs may also be feature requests if some or all of the following +are true: + +- Bugs with ``feature`` or similar in whiteboard or short description +- ``[RFE]`` in whiteboard, short description, or description +- Bugs not explicitly marked as a feature request, but appear to be + feature requests +- Bugs marked with ``feature`` keyword + +Initial Triage +-------------- + +Staff, contractors, and other contributors looking at new bugs in +*Firefox::Untriaged* and *::General* components should consider if a +bug, if not marked as a feature request, should be one, and if so: + +- Update the bug’s type to ``enhancement`` +- Determine which product and component the bug belongs to and update + it **or** + + - Use *needinfo* to ask a component’s triage owner or a module’s + owner where the request should go + +Product Manager Triage +---------------------- + +- The product manager for the component reviews bugs of type + ``enhancement`` + + - This review should be done a least weekly + +- Reassigns to another Product::Component if necessary **or** +- Determines next steps + + - Close bug as ``RESOLVED WONTFIX`` with comment as to why and + thanking submitter + - If bug is similar enough to work in roadmap, close bug as + ``RESOLVED DUPLICATE`` of the feature bug it is similar to + + - If there’s not a feature bug created already, then consider + making this bug the feature bug + + - Set type to ``enhancement`` + + - Set bug to ``P5`` priority with comment thanking submitter, and + explaining that the request will be considered for future roadmaps diff --git a/docs/bug-mgmt/policies/regressions-github.rst b/docs/bug-mgmt/policies/regressions-github.rst new file mode 100644 index 0000000000..1a3c6b2a4d --- /dev/null +++ b/docs/bug-mgmt/policies/regressions-github.rst @@ -0,0 +1,151 @@ +Regressions from GitHub +======================= + +Release Management and the weekly regression triage must be aware of the +status of all reported regressions in order to assure we are not +shipping known regressions in Firefox releases. + +If a team is using GitHub to manage their part of the Firefox project, +there’s a risk that those groups might not see a regression. + +We need an agreed to standard for how we keep track of these. + +Policy +------ + +*All Firefox components, even if their bugs are tracked off of Bugzilla, +must have a component in Bugzilla.* + +*If a regression bug is found in any of the release trains (Nightly, +Beta, Release, or ESR) and the bug is in a Firefox component which uses +an external repository, the regression must be tracked by a bug in +Bugzilla (whether or not the component in question uses an external +issue tracker).* + +*Unless approved by Release Management, any GitHub managed code with +open regressions will not be merged to mozilla-central. Even if the +regression is not code that has been previously merged into +mozilla-central.* + +*All Firefox code managed in GitHub which uses GitHub to manage +issues* `must use the shared +tags `__. + +Comments +~~~~~~~~ + +The bug **must** have the regression keyword. + +The bug **must** have release flags set. + +If the team works in an external bug tracker, then the Bugzilla bug +**must** reference, using the see-also field, the URL of the bug in the +external tracker. + +The bug **must not** be RESOLVED until the code from the external +repository containing the change set for the bug has landed in +mozilla-central. When the change set lands in mozilla-central, the +Bugzilla tracking bug should be set to RESOLVED FIXED and release status +flags should be updated to reflect the release trains the fix has been +landed or uplifted into. + +If the change set containing the patch for the regression is reverted +from mozilla-central, for any reason, then the tracking bug for the +regression **must** be set to REOPENED and the release status flags +updated accordingly. + +If the change set containing the patch for the bug is backed out, for +any reason, the bug must be reopened and the status flags on the +Bugzilla tracking bug updated. + +The team responsible for the component with the regression **should** +strive to create a patch for mozilla-central which contains the fix for +the bug alone, not a monolithic patch containing changes for several +other bugs or features. + +Landings of third-party libraries `must contain a manifest +file `__. + +Best Practices +-------------- + +You must file a regression bug in Bugzilla +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +*If the code with the regression has landed in mozilla-central, you must +file a regression bug.* + +Example +^^^^^^^ + +While using a release of Firefox (Nightly, Beta, Release, ESR) you run +across a bug. Upon research using MozRegression or other tools you find +that the bug was introduced in a change set imported from a component +whose code and issues are managed in GitHub. + +Actions to take +''''''''''''''' + +- Open a new bug in Bugzilla in appropriate component and add the + REGRESSION keyword +- Set affected status for the releases where the bug appears +- Open an issue in the corresponding GitHub project, put the Bugzilla + bug number in the title with the prefix ‘Bug’ (i.e. “Bug 99999: + Regression in foo”) +- Add the REGRESSION label to the new issue +- Add the link to the GitHub issue into the ‘See Also” field in the + Bugzilla bug + +Consequences +'''''''''''' + +*Until the regression is fixed or backed out of the GitHub repo, the +project cannot merge code into mozilla-central* + +Example +^^^^^^^ + +You import a development branch of a component managed in GitHub into +your copy of master. You find a bug and isolate it to the imported +branch. The code is managed in their own GitHub project, but bugs are +managed in Bugzilla. + +Actions to take +''''''''''''''' + +- Open a new bug in Bugzilla in appropriate component and add the + REGRESSION keyword +- Set affected status for the releases where the bug appears + +Consequences +'''''''''''' + +*Until the regression is fixed or backed out of the GitHub repo, the +project cannot merge code into mozilla-central* + +Do not file a regression bug in Bugzilla +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +*If the code with the regression has not landed in mozilla-central, you +do not need to file a bug.* + + +Example +^^^^^^^ + +You import a development branch of a component managed in GitHub into +your copy of master. You find a bug and isolate it to the imported +branch. The code and issues are managed in their own GitHub project. + + +Actions to take +''''''''''''''' + +- File new issue in the GitHub repository of the imported code. +- Label issue as REGRESSION + +Consequence +''''''''''' + +*Issue blocks merge of GitHub project with mozilla-central until +resolved or backed out.* diff --git a/docs/bug-mgmt/policies/triage-bugzilla.rst b/docs/bug-mgmt/policies/triage-bugzilla.rst new file mode 100644 index 0000000000..498e9955a8 --- /dev/null +++ b/docs/bug-mgmt/policies/triage-bugzilla.rst @@ -0,0 +1,278 @@ +Triage for Bugzilla +=================== + +Expectations +------------ + +All teams working on Firefox using either or both Mozilla-central and +Bugzilla are expected to follow the following process. + +What is a Triaged Bug +--------------------- + +The new definition of Triaged will be Firefox-related bugs of type +``defect`` where the component is not +``UNTRIAGED``, and a severity value not equal to ``--`` or ``N/A``. + +Bugs of type Task or Enhancement may have a severity of ``N/A``, +but defects must have a severity that is neither ``--`` or +``N/A``. + +Why Triage +---------- + +We want to make sure that we looked at every defect and a severity has +been defined. This way, we make sure that we did not miss any critical +issues during the development and stabilization cycles. + +Staying on top of the bugs in your component means: + +- You get ahead of critical regressions and crashes which could trigger + a point release if uncaught + + - And you don’t want to spend your holiday with the Release + Management team (not that they don’t like you) + +- Your bug queue is not overwhelming + + - Members of your team do not see the bug queue and get the + ‘wiggins’ + +Who Triages +----------- + +Engineering managers and directors are responsible for naming the +individuals responsible for triaging :ref:`all types of bugs ` in a component. + +We use Bugzilla to manage this. See the `list of triage +owners `__. + +If you need to change who is responsible for triaging a bug in a +component, please `file a bug against bugzilla.mozilla.org in the +Administration +component `__. +When a new component is created, a triage owner **must** be named. + +Rotating triage +~~~~~~~~~~~~~~~ + +Some components are monitored by a rotation of triagers. In those cases, +the triage owner should be seen as the person responsible for assuring +the component is triaged, but the work is done by the people in the +rotation. The `rotations are managed as +calendars `__. + +If you wish to set up a rotation for triaging one or more components, +contact the Bugzilla team on Slack (#bmo.) + +Firefox::General and Toolkit::General +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +Bugs in Firefox::General are fitted with Bug Bug’s model to see if +there’s another component with a high liklihood of fit, and if a +threshold confidence is achieved, the bug is moved to that component. + +Members of the community also review bugs in this component and try to +move them. + +What Do You Triage +------------------ + +As a triage owner the queries you should be following for your component +are: + +- All open bugs, in your components without a pending ``needinfo`` flag + which do not have a valid value of severity set +- All bugs with active review requests in your component which have not + been modified in five days +- All bugs with reviewed, but unlanded patches in your components +- All bugs with a needinfo request unanswered for more than 10 days + +There’s a tool with these queries to help you find bugs +https://mozilla.github.io/triage-center/ and the source is at +https://github.com/mozilla/triage-center/. + +If a bug is an enhancement it needs a priority set and a target release +or program milestone. These bugs are normally reviewed by product +managers. Enhancements can lead to release notes and QA needed that we +also need to know about + +If a bug is a task resulting in a changeset, release managers will need +to known when this work will be done. A task such as refactoring fragile +code can be risky. + +Weekly or More Frequently (depending on the component) find un-triaged +bugs in the components you triage. + +Decide the :ref:`Severity ` for each untriaged bug +(you can override what’s already been set.) + +These bugs are reviewed in the weekly Regression Triage meeting + +- Bugs of type ``defect`` with the ``regression`` keyword without + ``status-firefoxNN`` decisions +- Bugs of type ``defect`` with the ``regression`` keyword without + a regression range + +Automatic Bug Updates +~~~~~~~~~~~~~~~~~~~~~ + +When a bug is tracked for a release, i.e. the ``tracking_firefoxNN`` +flag is set to ``+`` or ``blocking`` triage decisions will be overridden, +or made as follows: + +- If a bug is tracked for or blocking beta, release or ESR, its + priority will be set to ``P1`` +- If a bug is tracked for or blocking nightly, its priority will be set + to ``P2`` + +Because bugs can be bumped in priority it’s essential that triage owners +review their +`P1 `__ +and +`P2 `__ +bugs frequently. + +Assumptions +~~~~~~~~~~~ + +If a bug's release status in Firefox version N was ``affected`` or ``wontfix``, +its Severity is ``S3`` or ``S4`` and its Priority is ``P3`` or lower (backlog,) +then its release status in Firefox version N+1, if the bug is still open, +is considered to be ``wontfix``. + +Questions and Edge Cases +------------------------ + +This bug is a feature request +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +Set the bug’s type to ``enhancement``, add the ``feature`` keyword if +relevant, and state to ``NEW``. Set the bug's Severity to ``N/A``. This +bug will be excluded from future triage queries. + +This bug is a task, not a defect +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +Set the bug’s type to ``task``, and state to ``NEW``. Set the bug's +Severity to ``N/A``. This bug will be excluded from future triage queries. + + +If you are not sure of a bug’s type, check :ref:`our rules for bug +types `. + +This bug’s state is ``UNCONFIRMED`` +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +Are there steps to reproduce? If not, needinfo the person who filed the +bug, requesting steps to reproduce. You are not obligated to wait +forever for a response, and bugs for which open requests for information +go unanswered can be ``RESOLVED`` as ``INCOMPLETE``. + +I need help reproducing the bug +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +Set a needinfo for the QA managers, Softvision project managers, or the +QA owner of the component of the bug. + +I don’t have enough information to make a decision +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +If you don’t have a reproduction or confirmation, or have questions +about how to proceed, ``needinfo`` the person who filed the bug, or +someone who can answer. + +The ``stalled`` keyword +~~~~~~~~~~~~~~~~~~~~~~~ + +The extreme case of not-enough-information is one which cannot be +answered with a ``needinfo`` request. The reporter has shared all they +know about the bug, we are out of strategies to take to resolve it, but +the bug should be kept open. + +Mark the bug as stalled by adding the ``stalled`` keyword to it. The +keyword will remove it from the list of bugs to be triaged. + +If a patch lands on a ``stalled`` bug, automation will remove the +keyword. Otherwise, when the ``keyword`` is removed, the bug will have +its priority reset to ``--`` and the components triage owner notified by +automation. + +Bugs which remain ``stalled`` for long periods of time should be +reviewed, and closed if necessary. + +Bug is in the wrong Component +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +If the bug has a Severity of ``S3``, ``S4``, or ``N/A`` move the what +you think is the correct component, or needinfo the person +responsible for the component to ask them. + +If the bug has a Severity of ``S1`` or ``S2`` then notify Release Management +and contact the triage owner of the component for which you think it belongs to. +We cannot lose track of a high severity bug because it is in the wrong component. + +My project is on GitHub +~~~~~~~~~~~~~~~~~~~~~~~ + +We have :ref:`a guide for GitHub projects to follow ` when +triaging. (Note: this guide needs updating.) + +Summary +------- + +Multiple times weekly +~~~~~~~~~~~~~~~~~~~~~ + +Use queries for the components you are responsible for in +https://mozilla.github.io/triage-center/ to find bugs in +need of triage. + +For each untriaged bug: + +- Assign a Severity +- **Do not** assign a ``defect`` a Severity of + ``N/A`` + +You can, but are not required to set the bug's :ref:`Priority `. + +Watch open needinfo flags +~~~~~~~~~~~~~~~~~~~~~~~~~ + +Don’t let open needinfo flags linger for more than two weeks. + +Close minor bugs with unresponded needinfo flags. + +Follow up on needinfo flag requests. + +The `Triage Center tool `__ will help you find these. + +End of Iteration/Release Cycle +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +Any open ``S1`` or ``S2`` bugs at the end of the release cycle +will require review by engineering and release management. A +policy on this is forthcoming. + +Optional +^^^^^^^^ + +(The guidelines on bug priority are under review.) + +Are there open P1s? Revisit their priority, +and move to them to the backlog (``P3``) or ``P2``. + +Are there ``P2`` bugs that should move to ``P1`` +for the next cycle? + +Are there ``P2`` you now know are lower priority, +move to ``P3``. + +Are there ``P3`` bugs you now know you won’t get to? +Either demote to ``P5`` (will accept patch) or +resolve as ``WONTFIX``. + +Getting help +------------ + +- Ask in #bug-handling on chat.mozilla.org diff --git a/docs/bug-mgmt/processes/accessibility-review.md b/docs/bug-mgmt/processes/accessibility-review.md new file mode 100644 index 0000000000..413eb8e6c6 --- /dev/null +++ b/docs/bug-mgmt/processes/accessibility-review.md @@ -0,0 +1,49 @@ +# Accessibility Review + +## Introduction +At Mozilla, accessibility is a fundamental part of our mission to ensure the +internet is "open and accessible to all," helping to empower people, regardless +of their abilities, to contribute to the common good. Accessibility Review is a +service provided by the Mozilla Accessibility team to review features and +changes to ensure they are accessible to and inclusive of people with +disabilities. + +## Do I Need Accessibility Review? +You should consider requesting accessibility review if you aren't certain +whether your change is accessible to people with disabilities. Accessibility +review is optional, but it is strongly encouraged if you are introducing new +user interface or are significantly redesigning existing user interface. + +## When Should I Request Accessibility Review? +Generally, it's best to request accessibility review as early as possible, even +during the product requirements or UI design stage. Particularly for more +complex user interfaces, accessibility is much easier when incorporated into the +design, rather than attempting to retro-fit accessibility after the +implementation is well underway. + +The accessibility team has developed the [Mozilla Accessibility Release +Guidelines](https://wiki.mozilla.org/Accessibility/Guidelines) which outline +what is needed to make user interfaces accessible. To make accessibility review +faster, you may wish to try to verify and implement these guidelines prior to +requesting accessibility review. + +The deadline for accessibility review requests is Friday of the first week of +nightly builds for the release in which the feature/change is expected to ship. +This is the same date as the PI Request deadline. + +## How Do I Request Accessibility Review? +You request accessibility review by setting the a11y-review flag to "requested" +on a bug in Bugzilla and filling in the template that appears in the comment +field. For features spanning several bugs, you may wish to file a new, dedicated +bug for the accessibility review. Otherwise, particularly for smaller changes, +you may do this on an existing bug. Note that if you file a new bug, you will +need to submit the bug and then edit it to set the flag. + +## Questions? +If you have any questions, please don't hesitate to contact the Accessibility +team: + +* \#accessibility on + [Matrix](https://matrix.to/#/!jmuErVonajdNMbgdeY:mozilla.org?via=mozilla.org&via=matrix.org) + or Slack +* Email: accessibility@mozilla.com diff --git a/docs/bug-mgmt/processes/doc-requests.rst b/docs/bug-mgmt/processes/doc-requests.rst new file mode 100644 index 0000000000..b725e10747 --- /dev/null +++ b/docs/bug-mgmt/processes/doc-requests.rst @@ -0,0 +1,44 @@ +User documentation requests +=========================== + +If you are working on a change (bugfix, enhancement, or feature) which +would benefit from user-facing documentation, please use the +``user-doc-firefox`` flag to request it. + +This flag can be modified by anyone with ``EDITBUGS`` privileges. + +.. figure:: /public/images/sumo-flag.png + :alt: Image of flag in tracking section of bug + + Image of flag in tracking section of bug + +The default value of the flag is ``---``. + +If the bug needs user-facing documentation, set the flag to +``docs-needed``. This flag will be monitored by the support.mozilla.org +(SUMO) team. + +Once the docs are ready to be published, set the flag to +``docs-completed``. + +If it’s determined that documentation is not need after setting the flag +to ``docs-needed``, update the flag to ``none-needed`` so we know that +it’s been reviewed. + +Summary +------- + +=========== == ============== +From To +=========== == ============== +— to none-needed +— to docs-needed +docs-needed to none-needed +docs-needed to docs-completed +=========== == ============== + +Notes +----- + +A flag is used instead of the old keywords because flags can be +restricted to a subset of products and components. diff --git a/docs/bug-mgmt/processes/fixing-security-bugs.rst b/docs/bug-mgmt/processes/fixing-security-bugs.rst new file mode 100644 index 0000000000..075cf7d65a --- /dev/null +++ b/docs/bug-mgmt/processes/fixing-security-bugs.rst @@ -0,0 +1,159 @@ +Fixing Security Bugs +==================== + +A bug has been reported as security-sensitive in Bugzilla and received a +security rating. + +If this bug is private - which is most likely for a reported security +bug - **the process for patching is slightly different than the usual +process for fixing a bug**. + +Here are security guidelines to follow if you’re involved in reviewing, +testing and landing a security patch. See +:ref:`Security Bug Approval Process` +for more details about how to request sec-approval and land the patch. + +Keeping private information private +----------------------------------- + +A security-sensitive bug in Bugzilla means that all information about +the bug except its ID number are hidden. This includes the title, +comments, reporter, assignee and CC’d people. + +A security-sensitive bug usually remains private until a fix is shipped +in a new release, **and after a certain amount of time to ensure that a +maximum number of users updated their version of Firefox**. Bugs are +usually made public after 6 months and a couple of releases. + +From the moment a security bug has been privately reported to the moment +a fix is shipped and the bug is set public, all information about that +bug need to be handled carefully in order to avoid an unmitigated +vulnerability to be known and exploited out there before we release a +fix (0-day). + +During a normal process, information about the nature of bug can be +accessed through: + +- Bug comments (Bugzilla, GitHub issue) +- Commit message (visible on Bugzilla, tree check-ins and test servers) +- Code comments +- Test cases +- Bug content can potentially be discussed on public IRC/Slack channels + and mailing list emails. + +When patching for a security bug, you’ll need to be mindful about what +type of information you share and where. + +In commit messages +~~~~~~~~~~~~~~~~~~ + +People are watching code check-ins, so we want to avoid sharing +information which would disclose or help finding a vulnerability too +easily before we shipped the fix to our users. This includes: + +- The **nature of the vulnerability** (overflow, use-after-free, XSS, + CSP bypass...) +- **Ways to trigger and exploit that vulnerability** + - In commit messages, code comments and test cases. +- The fact that a bug / commit is security-related: + + - **Trigger words** in the commit message or code comments such as "security", "exploitable" or the nature of a security vulnerability (overflow, use-after-free…) + - **Security approver’s name** in a commit message. +- The Firefox versions and components affected by the vulnerability. +- Patches with an obvious fix. + +In Bugzilla and other public channels +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +In addition to commits, you’ll need to be mindful of not disclosing +sensitive information about the bug in public places, such as Bugzilla: + +- **Do not add public bugs in the “duplicate”, “depends on”, “blocks” + or “see also” section if these bugs could give hints about the nature + of the security issue.** + + - Mention the bugs in comment of the private bug instead. +- Do not comment sensitive information in public related bugs. +- Also be careful about who you give bug access to: **double check + before CC’ing the wrong person or alias**. + +On IRC, Slack channels, GitHub issues, mailing lists: If you need to +discuss about a security bug, use a private channel (protected with a +password or with proper right access management) + +During Development +------------------ + +Testing sec-high and sec-critical bugs +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +Pushing to Try servers requires Level 1 Commit access but **content +viewing is publicly accessible**. + +As much as possible, **do not push to Try servers**. Testing should be +done locally before checkin in order to prevent public disclosing of the +bug. + +If you need to push to Try servers, make sure your tests don’t disclose +what the vulnerability is about or how to trigger it. Do not mention +anywhere it is security related. + +Obfuscating a security patch +~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +If your security patch looks obvious because of the code it contains +(e.g. a one-line fix), or if you really need to push to Try servers, +**consider integrating your security-related patch to non-security work +in the same area**. And/or pretend it is related to something else, like +some performance improvement or a correctness fix. **Definitely don't +include the bug number in the commit message.** This will help making +the security issue less easily identifiable. (The absolute ban against +"Security through Obscurity" is in relation to cryptographic systems. In +other situations you still can't *rely* on obscurity but it can +sometimes buy you a little time. In this context we need to get the +fixes into the hands of our users faster than attackers can weaponize +and deploy attacks and a little extra time can help.) + +Requesting sec-approval +~~~~~~~~~~~~~~~~~~~~~~~ + +See :ref:`Security Bug Approval Process` +for more details + +Landing tests +~~~~~~~~~~~~~ + +Tests can be landed **after the release has gone live** and **not before +at least 4 weeks following that release**. + +The exception is if a security issue has never been shipped in a release +build and has been fixed in all development branches. + +Making a security bug public +~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +This is the responsibility of the security management team. + +Essentials +---------- + +- **Do not disclose any information about the vulnerability before a + release with a fix has gone live for enough time for users to update + their software**. + + - This includes code comments, commit messages, tests, public + communication channels. + +- If any doubt: '''request sec-approval? ''' +- If any doubt: **needinfo security folks**. +- **If there’s no rating, assume the worst and treat the bug as + sec-critical**. + +Documentation & Contacts +------------------------ + +- :ref:`Normal process for submitting a patch ` +- `How to file a security bug `__ +- `Handling Mozilla security bugs (policy) `__ +- `Security Bug Approval Process `__ +- `Contacting the Security team(s) at Mozilla: `__ diff --git a/docs/bug-mgmt/processes/labels.rst b/docs/bug-mgmt/processes/labels.rst new file mode 100644 index 0000000000..0af3b6ef03 --- /dev/null +++ b/docs/bug-mgmt/processes/labels.rst @@ -0,0 +1,155 @@ +GitHub Metadata Recommendations +=============================== + +To have better consistency with code and task tracking among Mozilla +Central, Bugzilla, and GitHub, we request that you use a common set of +labels in your projects. Benefits of improved consistency in our +conventions include: + +- Consistency makes measurement of processes simpler across the + organization +- Consistency makes it easier to write re-usable process tools +- Consistency increases clarity for those than need to work across + different repositories and bug trackers +- Consistency reduces friction around engineering mobility between + projects + +We recommend creating sets of labels in your project to do this. + +Bug types +--------- + +In Bugzilla bugs are distinguished by type: ``defect``, ``enhancement``, +and ``tasks``. Use a label to make this distinction in your project. + +Statuses +-------- + +Bugs in GitHub issues have two states: closed and open. Bugzilla has a +richer set of states. + +When you close a bug, add a label indicating `the +resolution `__. + +- ``fixed`` + + - A change set for the bug has been landed in Mozilla-Central + - A GitHub issue could be closed, but the change set has not + landed so it would be still considered open from the + Bugzilla point of view + +- ``invalid`` + + - The problem described is not a bug. + +- ``incomplete`` + + - The problem is vaguely described with no steps to reproduce, or is + a support request. + +- ``wontfix`` + + - The problem described is a bug which will never be fixed. + +- ``duplicate`` + + - The problem is a duplicate of an existing bug. Be sure to link the + bug this is a duplicate of. + +- ``worksforme`` + + - All attempts at reproducing this bug were futile, and reading the + code produces no clues as to why the described behavior would + occur. + +Severities (Required) +--------------------- + +The triage process for Firefox bugs in Bugzilla requires a non default +value of a bug's :ref:`Severity (definitions) `. + +Release Status Flags +------------------------------- + +Open Firefox bugs may also have :ref:`status flags ` +(``status_firefoxNN``) set for Nightly, Beta, Release, or ESR. + +Priorities +---------- + +Firefox projects in Bugzilla can use the :ref:`priority field ` +to indicate when a bug will be worked on. + +Keywords +-------- + +In GitHub issues metadata is either a label or the bug’s open/closed +state. + +Some Bugzilla metadata behaves like labels, but you need to be careful +with how you use it in order not to confuse QA. + +Regressions +~~~~~~~~~~~ + +In Bugzilla, the ``regression`` keyword indicates a regression in +existing behavior introduced by a code change. + +When a bug is labeled as a regression in GitHub does it imply the +regression is in the code module in GitHub, or the module that’s landed +in Mozilla Central? Using the label ``regression-internal`` will signal +QA that the regression is internal to your development cycle, and not +one introduced into the Mozilla Central tree. + +If it is not clear which pull request caused the regression, add the +``regressionwindow-wanted`` label. + +Other Keywords +~~~~~~~~~~~~~~ + +Other useful labels include ``enhancement`` to distinguish feature +requests, and ``good first issue`` to signal to contributors (`along +with adequate +documentation `__.) + +Summary +------- + +To represent Bugzilla fields, use labels following this scheme. + +- Bug types + + - ``defect``, ``enhancement``, ``task`` + +- Resolution statuses + + - ``invalid``, ``duplicate``, ``incomplete``, ``worksforme``, + ``wontfix`` + +- Regressions + + - ``regression``, ``regressionwindow-wanted``, + ``regression-internal`` + + +- :ref:`Severity ` (required) + + - ``S1``, ``S2``, ``S3``, ``S4``, ``N/A`` (reserved for bugs + of type ``task`` or ``enhancement``) + +- :ref:`Status flags ` + + - ``status_firefoxNN:`` + (example ``status_firefox77:affected``) + +- :ref:`Priority ` + + - ``P1``, ``P2``, ``P3``, ``P5`` + +- Other keywords + + - ``good first bug``, ``perf``, &etc. + + +You may already have a set of tags, so do an edit to convert them +or use `the GitHub settings app `__. diff --git a/docs/bug-mgmt/processes/regressions.rst b/docs/bug-mgmt/processes/regressions.rst new file mode 100644 index 0000000000..991771f38d --- /dev/null +++ b/docs/bug-mgmt/processes/regressions.rst @@ -0,0 +1,64 @@ +How to Mark Regressions +======================= + +Regressions +----------- + +For regression bugs in Mozilla-Central, our policy is to tag the bug as +a regression, identify the commits which caused the regression, then +mark the bugs associated with those commits as causing the regression. + +What is a regression? +--------------------- + +A regression is a bug (in our scheme a ``defect``) introduced by a +`changeset `__. + +- Bug 101 *fixes* Bug 100 with Change Set A +- Bug 102 *reported which describes previously correct behavior now not + happening* +- Bug 102 *investigated and found to be introduced by Change Set A* + +Marking a Regression Bug +------------------------ + +These things are true about regressions: + +- **Bug Type** is ``defect`` +- **Keywords** include ``regression`` +- **Status_FirefoxNN** is ``affected`` for each version (in current + nightly, beta, and release) of Firefox in which the bug was found +- The bug’s description covers previously working behavior which is no + longer working [ed. I need a better phrase for this] + +Until the change set which caused the regression has been found through +`mozregression `__ or another +bisection tool, the bug should also have the ``regressionwindow-wanted`` +keyword. + +Once the change set which caused the regression has been identified, +remove the ``regressionwindow-wanted`` keyword and set the **Regressed +By** field to the id of the bug associated with the change set. + +Setting the **Regressed By** field will update the **Regresses** field +in the other bug. + +Set a needinfo for the author of the regressing patch asking them to fix +or revert the regression. + +Previous Method +--------------- + +Previously we over-loaded the **Blocks** and **Blocked By** fields to +track the regression, setting **Blocks** to the id of the bug associated +with the change set causing the regression, and using the +``regression``, ``regressionwindow-wanted`` keywords and the status +flags as described above. + +This made it difficult to understand what was a dependency and what was +a regression when looking at dependency trees in Bugzilla. + +FAQs +---- + +*To be written* diff --git a/docs/bug-mgmt/processes/security-approval.rst b/docs/bug-mgmt/processes/security-approval.rst new file mode 100644 index 0000000000..e8148525ab --- /dev/null +++ b/docs/bug-mgmt/processes/security-approval.rst @@ -0,0 +1,219 @@ +Security Bug Approval Process +============================= + +How to fix a core-security bug in Firefox - developer guidelines +---------------------------------------------------------------- + +Follow these security guidelines if you’re involved in reviewing, +testing and landing a security patch: +:ref:`Fixing Security Bugs`. + +Purpose: don't 0-day ourselves +------------------------------ + +People watch our check-ins. They may be able to start exploiting our +users before we can get an update out to them if + +- the patch is an obvious security fix (bounds check, kungFuDeathGrip, + etc.) +- the check-in comment says "security fix" or includes trigger words + like "exploitable", "vulnerable", "overflow", "injection", "use after + free", etc. +- comments in the code mention those types of things or how someone + could abuse the bug +- the check-in contains testcases that show exactly how to trigger the + vulnerability + +Principle: assume the worst +--------------------------- + +- If there's no rating we assume it could be critical +- If we don't know the regression range we assume it needs porting to + all supported branches + +Process for Security Bugs (Developer Perspective) +------------------------------------------------- + +Filing / Managing Bugs +~~~~~~~~~~~~~~~~~~~~~~ + +- Try whenever possible to file security bugs marked as such when + filing, instead of filing them as open bugs and then closing later. + This is not always possible, but attention to this, especially when + filing from crash-stats, is helpful. +- Avoid linking it to non-security bugs with Blocks, Depends, + Regressions, or See Also, especially if those bugs may give a hint to + the sort of security issue involved. Mention the bug in a comment on + the security bug instead. We can always fill in the links later after + the fix has shipped. + +Developing the Patch +~~~~~~~~~~~~~~~~~~~~ + +- Comments in the code should not mention a security issue is being + fixed. Don’t paint a picture or an arrow pointing to security issues + any more than the code changes already do. +- Avoid linking it to non-security bugs with Blocks, Depends, or See + Also, especially if those bugs may give a hint to the sort of + security issue involved. Mention the bug in a comment on the security + bug instead. We can always fill in the links later after the fix has + shipped. +- Do not push to Try servers if possible: this exposes the security + issues for these critical and high rated bugs to public viewing. In + an ideal case, testing of patches is done locally before final + check-in to mozilla-central. +- If pushing to Try servers is necessary, **do not include the bug + number in the patch**. Ideally, do not include tests in the push as + the tests can illustrate the exact nature of the security problem + frequently. +- If you must push to Try servers, with or without tests, try to + obfuscate what this patch is for. Try to push it with other, + non-security work, in the same area. + +Request review of the patch in the same process as normal. After the +patch has received an r+ you will request sec-approval. See +:ref:`Fixing Security Bugs` +for more examples/details of these points. + +On Requesting sec-approval +~~~~~~~~~~~~~~~~~~~~~~~~~~ + +For security bugs with no sec- severity rating assume the worst and +follow the rules for sec-critical. During the sec-approval process we +will notice it has not been rated and rate it during the process. + +Core-security bug fixes can be landed by a developer without any +explicit approval if: + +| **A)** The bug has a sec-low, sec-moderate, sec-other, or sec-want + rating. +|    **or** +| **B)** The bug is a recent regression on mozilla-central. This means + +- A specific regressing check-in has been identified +- The developer can (**and has**) marked the status flags for ESR, + Beta, and Aurora as "unaffected" +- We have not shipped this vulnerability in anything other than a + nightly build + +If it meets the above criteria, check that patch in. + +Otherwise, if the bug has a patch \*and\* is sec-high or sec-critical, +the developer should prepare the patch for sec-approval. This entails: + +- Commit should occur without specific mention of security, security + bugs, or sec-approvers if possible. While comprehensive commit + messages are generally encouraged; they should be omitted for + security bugs and instead be posted in the bug (which will eventually + become public.) +- Separate out tests into a separate commit. **Do not commit tests when + checking in** when the security bug fix is initially checked-in. + **Remember we don’t want to 0-day ourselves!** + + - Tests should only be checked in later, after an official Firefox + release that contains the fix has gone live and not for at least + four weeks following that release. For example, if Firefox 53 + contains a fix for a security issue that affects the world and is + then fixed in 54, tests for this fix should not be checked in + until four weeks after 54 goes live. The exception to this is if + there is a security issue that hasn’t shipped in a release build + and it is being fixed on multiple development branches (such as + mozilla-central and beta). Since the security problem was never + released to the world, once the bug is fixed in all affected + places, tests can be checked in to the various branches. + - There are two main techniques for remembering to check in the + tests later: + + - clone the sec bug into a hidden "task" bug "land tests for bug + xxxxx" and assign to yourself. It should get a "sec-other" + keyword rating. + - Or, set the "in-testsuite" flag to "?", and later set it to "+" + when the tests get checked in. + +Following that, set the sec-approval flag to '?' on the patch when it is +ready to be checked into mozilla-central (or elsewhere if it is branch +only). + +If developers are unsure about a bug and it has a patch ready, just +request sec-approval anyway and move on. Don't overthink it! + +An automatic nomination comment will be added to bugzilla when +sec-approval is set to '?'. The questions in this need to be filled out +as best as possible when sec-approval is requested for the patch. + +It is as follows (courtesy of Dan Veditz):: + + [Security approval request comment] + How easily can the security issue be deduced from the patch? + Do comments in the patch, the check-in comment, or tests included in + the patch paint a bulls-eye on the security problem? + Which older supported branches are affected by this flaw? + If not all supported branches, which bug introduced the flaw? + Do you have backports for the affected branches? If not, how + different, hard to create, and risky will they be? + How likely is this patch to cause regressions; how much testing does + it need? + +This is similar to the ESR approval nomination form and is meant to help +us evaluate the risks around approving the patch for checkin. + +When the bug is approved for landing, the sec-approval flag will be set +to '+' with a comment from the approver to land the patch. At that +point, land it according to instructions provided.. + +This will allow us to control when we can land security bugs without +exposing them too early and to make sure they get landed on the various +branches. + +If you have any questions or are unsure about anything in this document +contact us on Slack in the #security channel or the current +sec-approvers Dan Veditz and Tom Ritter. + +Process for Security Bugs (sec-approver Perspective) +---------------------------------------------------- + +The security assurance team and release management will have their own +process for approving bugs: + +#. The Security assurance team goes through sec-approval ? bugs daily + and approves low risk fixes for central (if early in cycle). + Developers can also ping the Security Assurance Team (specifically + Tom Ritter & Dan Veditz) in #security on Slack when important. + + #. If a bug lacks a security-rating one should be assigned - possibly + in coordination with the (other member of) the Security Assurance + Team + +#. Security team marks tracking flags to ? for all affected versions + when approved for central. (This allows release management to decide + whether to uplift to branches just like always.) +#. Weekly security/release management triage meeting goes through + sec-approval + and ? bugs where beta and ESR is affected, ? bugs with + higher risk (sec-high and sec-critical), or ? bugs near end of cycle. + +Options for sec-approval including a logical combination of the +following: + +- Separate out the test and comments in the code into a followup commit + we will commit later. +- Remove the commit message and place it in the bug or comments in a + followup commit. +- Please land it bundled in with another commit +- Land today +- Land today, land the tests after +- Land closer to the release date +- Land in Nightly to assess stability +- Land today and request uplift to all branches +- Request uplift to all branches and we'll land as close to shipping as + permitted +- Chemspill time + +The decision process for which of these to choose is perceived risk on +multiple axes: + +- ease of exploitation +- reverse engineering risk +- stability risk + +The most common choice is: not much stability risk, not an immediate RE +risk, moderate to high difficulty of exploitation: "land whenever" \ No newline at end of file diff --git a/docs/bug-mgmt/processes/shared-bug-queues.rst b/docs/bug-mgmt/processes/shared-bug-queues.rst new file mode 100644 index 0000000000..dc2df9bbf9 --- /dev/null +++ b/docs/bug-mgmt/processes/shared-bug-queues.rst @@ -0,0 +1,34 @@ +Shared Bug Queues +================= + +Reviewers for change sets can be suggested at the product and component +level, but only the person who has been asked to review code will be +notified. + +Realizing that Bugzilla users can *watch* other users, `Chris +Cooper `__ came up with the idea +of having `a shared reviews alias for review +requests `__. + +If you want to watch a particular part of the tree in Mozilla Central, +then `use the Herald +tool `__. + +Process +------- + +1. Create a new bugzilla.mozilla.com account for an address which can + receive mail. + Use the ``name+extension@domain.tld`` trick such as + ``jmozillian+reviews@mozilla.com`` to create a unique address +2. Respond to the email sent by Bugzilla and set a password on the + account +3. `Open a bug `__ to convert the account to a + bot and make it the shared review queue for your component +4. BMO administrator updates the email address of the new account to the + ``@mozilla.bugs`` address +5. BMO administrator updates the default reviewer for the component + requested and sets it to the shared review account +6. Reviewers `follow the shared review account in + bugzilla `__ +7. Reviewers get notified when shared review account is ``r?``\ ed diff --git a/docs/code-quality/coding-style/coding_style_cpp.rst b/docs/code-quality/coding-style/coding_style_cpp.rst new file mode 100644 index 0000000000..09319cfd12 --- /dev/null +++ b/docs/code-quality/coding-style/coding_style_cpp.rst @@ -0,0 +1,867 @@ +================ +C++ Coding style +================ + + +This document attempts to explain the basic styles and patterns used in +the Mozilla codebase. New code should try to conform to these standards, +so it is as easy to maintain as existing code. There are exceptions, but +it's still important to know the rules! + +This article is particularly for those new to the Mozilla codebase, and +in the process of getting their code reviewed. Before requesting a +review, please read over this document, making sure that your code +conforms to recommendations. + +.. container:: blockIndicator warning + + The Firefox code base adopts parts of the `Google Coding style for C++ + code `__, but not all of its rules. + A few rules are followed across the code base, others are intended to be + followed in new or significantly revised code. We may extend this list in the + future, when we evaluate the Google Coding Style for C++ Code further and/or update + our coding practices. However, the plan is not to adopt all rules of the Google Coding + Style for C++ Code. Some rules are explicitly unlikely to be adopted at any time. + + Followed across the code base: + + - `Formatting `__, + except for subsections noted here otherwise + - `Implicit Conversions `__, + which is enforced by a custom clang-plugin check, unless explicitly overridden using + ``MOZ_IMPLICIT`` + + Followed in new/significantly revised code: + + - `Include guards `__ + + Unlikely to be ever adopted: + + - `Forward declarations `__ + - `Formatting/Conditionals `__ + w.r.t. curly braces around inner statements, we require them in all cases where the + Google style allows to leave them out for single-line conditional statements + + This list reflects the state of the Google Google Coding Style for C++ Code as of + 2020-07-17. It may become invalid when the Google modifies its Coding Style. + + +Formatting code +--------------- + +Formatting is done automatically via clang-format, and controlled via in-tree +configuration files. See :ref:`Formatting C++ Code With clang-format` +for more information. + +Unix-style linebreaks (``\n``), not Windows-style (``\r\n``). You can +convert patches, with DOS newlines to Unix via the ``dos2unix`` utility, +or your favorite text editor. + +Static analysis +--------------- + +Several of the rules in the Google C++ coding styles and the additions mentioned below +can be checked via clang-tidy (some rules are from the upstream clang-tidy, some are +provided via a mozilla-specific plugin). Some of these checks also allow fixes to +be automatically applied. + +``mach static-analysis`` provides a convenient way to run these checks. For example, +for the check called ``google-readability-braces-around-statements``, you can run: + +.. code-block:: shell + + ./mach static-analysis check --checks="-*,google-readability-braces-around-statements" --fix + +It may be necessary to reformat the files after automatically applying fixes, see +:ref:`Formatting C++ Code With clang-format`. + +Additional rules +---------------- + +*The norms in this section should be followed for new code. For existing code, +use the prevailing style in a file or module, ask the owner if you are +in another team's codebase or it's not clear what style to use.* + + + + +Control structures +~~~~~~~~~~~~~~~~~~ + +Always brace controlled statements, even a single-line consequent of +``if else else``. This is redundant, typically, but it avoids dangling +else bugs, so it's safer at scale than fine-tuning. + +Examples: + +.. code-block:: cpp + + if (...) { + } else if (...) { + } else { + } + + while (...) { + } + + do { + } while (...); + + for (...; ...; ...) { + } + + switch (...) { + case 1: { + // When you need to declare a variable in a switch, put the block in braces. + int var; + break; + } + case 2: + ... + break; + default: + break; + } + +``else`` should only ever be followed by ``{`` or ``if``; i.e., other +control keywords are not allowed and should be placed inside braces. + +.. note:: + + For this rule, clang-tidy provides the ``google-readability-braces-around-statements`` + check with autofixes. + + +C++ namespaces +~~~~~~~~~~~~~~ + +Mozilla project C++ declarations should be in the ``mozilla`` +namespace. Modules should avoid adding nested namespaces under +``mozilla``, unless they are meant to contain names which have a high +probability of colliding with other names in the code base. For example, +``Point``, ``Path``, etc. Such symbols can be put under +module-specific namespaces, under ``mozilla``, with short +all-lowercase names. Other global namespaces besides ``mozilla`` are +not allowed. + +No ``using`` directives are allowed in header files, except inside class +definitions or functions. (We don't want to pollute the global scope of +compilation units that use the header file.) + +.. note:: + + For parts of this rule, clang-tidy provides the ``google-global-names-in-headers`` + check. It only detects ``using namespace`` directives in the global namespace. + + +``using namespace ...;`` is only allowed in ``.cpp`` files after all +``#include``\ s. Prefer to wrap code in ``namespace ... { ... };`` +instead, if possible. ``using namespace ...;``\ should always specify +the fully qualified namespace. That is, to use ``Foo::Bar`` do not +write ``using namespace Foo; using namespace Bar;``, write +``using namespace Foo::Bar;`` + +Use nested namespaces (ex: ``namespace mozilla::widget {`` + +.. note:: + + clang-tidy provides the ``modernize-concat-nested-namespaces`` + check with autofixes. + + +Anonymous namespaces +~~~~~~~~~~~~~~~~~~~~ + +We prefer using ``static``, instead of anonymous C++ namespaces. This may +change once there is better debugger support (especially on Windows) for +placing breakpoints, etc. on code in anonymous namespaces. You may still +use anonymous namespaces for things that can't be hidden with ``static``, +such as types, or certain objects which need to be passed to template +functions. + + +C++ classes +~~~~~~~~~~~~ + +.. code-block:: cpp + + namespace mozilla { + + class MyClass : public A + { + ... + }; + + class MyClass + : public X + , public Y + { + public: + MyClass(int aVar, int aVar2) + : mVar(aVar) + , mVar2(aVar2) + { + ... + } + + // Special member functions, like constructors, that have default bodies + // should use '= default' annotation instead. + MyClass() = default; + + // Unless it's a copy or move constructor or you have a specific reason to allow + // implicit conversions, mark all single-argument constructors explicit. + explicit MyClass(OtherClass aArg) + { + ... + } + + // This constructor can also take a single argument, so it also needs to be marked + // explicit. + explicit MyClass(OtherClass aArg, AnotherClass aArg2 = AnotherClass()) + { + ... + } + + int LargerFunction() + { + ... + ... + } + + private: + int mVar; + }; + + } // namespace mozilla + +Define classes using the style given above. + +.. note:: + + For the rule on ``= default``, clang-tidy provides the ``modernize-use-default`` + check with autofixes. + + For the rule on explicit constructors and conversion operators, clang-tidy + provides the ``mozilla-implicit-constructor`` check. + +Existing classes in the global namespace are named with a short prefix +(For example, ``ns``) as a pseudo-namespace. + + +Methods and functions +~~~~~~~~~~~~~~~~~~~~~ + + +C/C++ +^^^^^ + +In C/C++, method names should use ``UpperCamelCase``. + +Getters that never fail, and never return null, are named ``Foo()``, +while all other getters use ``GetFoo()``. Getters can return an object +value, via a ``Foo** aResult`` outparam (typical for an XPCOM getter), +or as an ``already_AddRefed`` (typical for a WebIDL getter, +possibly with an ``ErrorResult& rv`` parameter), or occasionally as a +``Foo*`` (typical for an internal getter for an object with a known +lifetime). See `the bug 223255 `_ +for more information. + +XPCOM getters always return primitive values via an outparam, while +other getters normally use a return value. + +Method declarations must use, at most, one of the following keywords: +``virtual``, ``override``, or ``final``. Use ``virtual`` to declare +virtual methods, which do not override a base class method with the same +signature. Use ``override`` to declare virtual methods which do +override a base class method, with the same signature, but can be +further overridden in derived classes. Use ``final`` to declare virtual +methods which do override a base class method, with the same signature, +but can NOT be further overridden in the derived classes. This should +help the person reading the code fully understand what the declaration +is doing, without needing to further examine base classes. + +.. note:: + + For the rule on ``virtual/override/final``, clang-tidy provides the + ``modernize-use-override`` check with autofixes. + + +Operators +~~~~~~~~~ + +The unary keyword operator ``sizeof``, should have its operand parenthesized +even if it is an expression; e.g. ``int8_t arr[64]; memset(arr, 42, sizeof(arr));``. + + +Literals +~~~~~~~~ + +Use ``\uXXXX`` unicode escapes for non-ASCII characters. The character +set for XUL, DTD, script, and properties files is UTF-8, which is not easily +readable. + + +Prefixes +~~~~~~~~ + +Follow these naming prefix conventions: + + +Variable prefixes +^^^^^^^^^^^^^^^^^ + +- k=constant (e.g. ``kNC_child``). Not all code uses this style; some + uses ``ALL_CAPS`` for constants. +- g=global (e.g. ``gPrefService``) +- a=argument (e.g. ``aCount``) +- C++ Specific Prefixes + + - s=static member (e.g. ``sPrefChecked``) + - m=member (e.g. ``mLength``) + - e=enum variants (e.g. ``enum Foo { eBar, eBaz }``). Enum classes + should use ``CamelCase`` instead (e.g. + ``enum class Foo { Bar, Baz }``). + + +Global functions/macros/etc +^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +- Macros begin with ``MOZ_``, and are all caps (e.g. + ``MOZ_WOW_GOODNESS``). Note that older code uses the ``NS_`` prefix; + while these aren't being changed, you should only use ``MOZ_`` for + new macros. The only exception is if you're creating a new macro, + which is part of a set of related macros still using the old ``NS_`` + prefix. Then you should be consistent with the existing macros. + + +Error Variables +^^^^^^^^^^^^^^^ + +- Local variables that are assigned ``nsresult`` result codes should be named ``rv`` + (i.e., e.g., not ``res``, not ``result``, not ``foo``). `rv` should not be + used for bool or other result types. +- Local variables that are assigned ``bool`` result codes should be named `ok`. + + +C/C++ practices +--------------- + +- **Have you checked for compiler warnings?** Warnings often point to + real bugs. `Many of them `__ + are enabled by default in the build system. +- In C++ code, use ``nullptr`` for pointers. In C code, using ``NULL`` + or ``0`` is allowed. + +.. note:: + + For the C++ rule, clang-tidy provides the ``modernize-use-nullptr`` check + with autofixes. + +- Don't use ``PRBool`` and ``PRPackedBool`` in C++, use ``bool`` + instead. +- For checking if a ``std`` container has no items, don't use + ``size()``, instead use ``empty()``. +- When testing a pointer, use ``(!myPtr)`` or ``(myPtr)``; + don't use ``myPtr != nullptr`` or ``myPtr == nullptr``. +- Do not compare ``x == true`` or ``x == false``. Use ``(x)`` or + ``(!x)`` instead. ``if (x == true)`` may have semantics different from + ``if (x)``! + +.. note:: + + clang-tidy provides the ``readability-simplify-boolean-expr`` check + with autofixes that checks for these and some other boolean expressions + that can be simplified. + +- In general, initialize variables with ``nsFoo aFoo = bFoo,`` and not + ``nsFoo aFoo(bFoo)``. + + - For constructors, initialize member variables with : ``nsFoo + aFoo(bFoo)`` syntax. + +- To avoid warnings created by variables used only in debug builds, use + the + `DebugOnly `__ + helper when declaring them. +- You should `use the static preference + API `__ for + working with preferences. +- One-argument constructors, that are not copy or move constructors, + should generally be marked explicit. Exceptions should be annotated + with ``MOZ_IMPLICIT``. +- Use ``char32_t`` as the return type or argument type of a method that + returns or takes as argument a single Unicode scalar value. (Don't + use UTF-32 strings, though.) +- Forward-declare classes in your header files, instead of including + them, whenever possible. For example, if you have an interface with a + ``void DoSomething(nsIContent* aContent)`` function, forward-declare + with ``class nsIContent;`` instead of ``#include "nsIContent.h"`` +- Include guards are named per the Google coding style and should not + include a leading ``MOZ_`` or ``MOZILLA_``. For example + ``dom/media/foo.h`` would use the guard ``DOM_MEDIA_FOO_H_``. +- Avoid the usage of ``typedef``, instead, please use ``using`` instead. + +.. note:: + + For parts of this rule, clang-tidy provides the ``modernize-use-using`` + check with autofixes. + + +COM and pointers +---------------- + +- Use ``nsCOMPtr<>`` + If you don't know how to use it, start looking in the code for + examples. The general rule, is that the very act of typing + ``NS_RELEASE`` should be a signal to you to question your code: + "Should I be using ``nsCOMPtr`` here?". Generally the only valid use + of ``NS_RELEASE`` is when you are storing refcounted pointers in a + long-lived datastructure. +- Declare new XPCOM interfaces using `XPIDL `__, so they + will be scriptable. +- Use `nsCOMPtr `__ for strong references, and + `nsWeakPtr `__ for weak references. +- Don't use ``QueryInterface`` directly. Use ``CallQueryInterface`` or + ``do_QueryInterface`` instead. +- Use `Contract + IDs `__, + instead of CIDs with ``do_CreateInstance``/``do_GetService``. +- Use pointers, instead of references for function out parameters, even + for primitive types. + + +IDL +--- + + +Use leading-lowercase, or "interCaps" +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +When defining a method or attribute in IDL, the first letter should be +lowercase, and each following word should be capitalized. For example: + +.. code-block:: cpp + + long updateStatusBar(); + + +Use attributes wherever possible +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +Whenever you are retrieving or setting a single value, without any +context, you should use attributes. Don't use two methods when you could +use an attribute. Using attributes logically connects the getting and +setting of a value, and makes scripted code look cleaner. + +This example has too many methods: + +.. code-block:: cpp + + interface nsIFoo : nsISupports + { + long getLength(); + void setLength(in long length); + long getColor(); + }; + +The code below will generate the exact same C++ signature, but is more +script-friendly. + +.. code-block:: cpp + + interface nsIFoo : nsISupports + { + attribute long length; + readonly attribute long color; + }; + + +Use Java-style constants +~~~~~~~~~~~~~~~~~~~~~~~~ + +When defining scriptable constants in IDL, the name should be all +uppercase, with underscores between words: + +.. code-block:: cpp + + const long ERROR_UNDEFINED_VARIABLE = 1; + + +See also +~~~~~~~~ + +For details on interface development, as well as more detailed style +guides, see the `Interface development +guide `__. + + +Error handling +-------------- + + +Check for errors early and often +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +Every time you make a call into an XPCOM function, you should check for +an error condition. You need to do this even if you know that call will +never fail. Why? + +- Someone may change the callee in the future to return a failure + condition. +- The object in question may live on another thread, another process, + or possibly even another machine. The proxy could have failed to make + your call in the first place. + +Also, when you make a new function which is failable (i.e. it will +return a ``nsresult`` or a ``bool`` that may indicate an error), you should +explicitly mark the return value should always be checked. For example: + +:: + + // for IDL. + [must_use] nsISupports + create(); + + // for C++, add this in *declaration*, do not add it again in implementation. + [[nodiscard]] nsresult + DoSomething(); + +There are some exceptions: + +- Predicates or getters, which return ``bool`` or ``nsresult``. +- IPC method implementation (For example, ``bool RecvSomeMessage()``). +- Most callers will check the output parameter, see below. + +.. code-block:: cpp + + nsresult + SomeMap::GetValue(const nsString& key, nsString& value); + +If most callers need to check the output value first, then adding +``[[nodiscard]]`` might be too verbose. In this case, change the return value +to void might be a reasonable choice. + +There is also a static analysis attribute ``MOZ_MUST_USE_TYPE``, which can +be added to class declarations, to ensure that those declarations are +always used when they are returned. + + +Use the NS_WARN_IF macro when errors are unexpected. +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +The ``NS_WARN_IF`` macro can be used to issue a console warning, in debug +builds if the condition fails. This should only be used when the failure +is unexpected and cannot be caused by normal web content. + +If you are writing code which wants to issue warnings when methods fail, +please either use ``NS_WARNING`` directly, or use the new ``NS_WARN_IF`` macro. + +.. code-block:: cpp + + if (NS_WARN_IF(somethingthatshouldbefalse)) { + return NS_ERROR_INVALID_ARG; + } + + if (NS_WARN_IF(NS_FAILED(rv))) { + return rv; + } + +Previously, the ``NS_ENSURE_*`` macros were used for this purpose, but +those macros hide return statements, and should not be used in new code. +(This coding style rule isn't generally agreed, so use of ``NS_ENSURE_*`` +can be valid.) + + +Return from errors immediately +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +In most cases, your knee-jerk reaction should be to return from the +current function, when an error condition occurs. Don't do this: + +.. code-block:: cpp + + rv = foo->Call1(); + if (NS_SUCCEEDED(rv)) { + rv = foo->Call2(); + if (NS_SUCCEEDED(rv)) { + rv = foo->Call3(); + } + } + return rv; + +Instead, do this: + +.. code-block:: cpp + + rv = foo->Call1(); + if (NS_FAILED(rv)) { + return rv; + } + + rv = foo->Call2(); + if (NS_FAILED(rv)) { + return rv; + } + + rv = foo->Call3(); + if (NS_FAILED(rv)) { + return rv; + } + +Why? Error handling should not obfuscate the logic of the code. The +author's intent, in the first example, was to make 3 calls in +succession. Wrapping the calls in nested if() statements, instead +obscured the most likely behavior of the code. + +Consider a more complicated example to hide a bug: + +.. code-block:: cpp + + bool val; + rv = foo->GetBooleanValue(&val); + if (NS_SUCCEEDED(rv) && val) { + foo->Call1(); + } else { + foo->Call2(); + } + +The intent of the author, may have been, that ``foo->Call2()`` would only +happen when val had a false value. In fact, ``foo->Call2()`` will also be +called, when ``foo->GetBooleanValue(&val)`` fails. This may, or may not, +have been the author's intent. It is not clear from this code. Here is +an updated version: + +.. code-block:: cpp + + bool val; + rv = foo->GetBooleanValue(&val); + if (NS_FAILED(rv)) { + return rv; + } + if (val) { + foo->Call1(); + } else { + foo->Call2(); + } + +In this example, the author's intent is clear, and an error condition +avoids both calls to ``foo->Call1()`` and ``foo->Call2();`` + +*Possible exceptions:* Sometimes it is not fatal if a call fails. For +instance, if you are notifying a series of observers that an event has +fired, it might be trivial that one of these notifications failed: + +.. code-block:: cpp + + for (size_t i = 0; i < length; ++i) { + // we don't care if any individual observer fails + observers[i]->Observe(foo, bar, baz); + } + +Another possibility, is you are not sure if a component exists or is +installed, and you wish to continue normally, if the component is not +found. + +.. code-block:: cpp + + nsCOMPtr service = do_CreateInstance(NS_MYSERVICE_CID, &rv); + // if the service is installed, then we'll use it. + if (NS_SUCCEEDED(rv)) { + // non-fatal if this fails too, ignore this error. + service->DoSomething(); + + // this is important, handle this error! + rv = service->DoSomethingImportant(); + if (NS_FAILED(rv)) { + return rv; + } + } + + // continue normally whether or not the service exists. + + +Strings +------- + +.. note:: + + This section overlaps with the more verbose advice given in + `Internal strings `__. + These should eventually be merged. For now, please refer to that guide for + more advice. + +- String arguments to functions should be declared as ``[const] nsA[C]String&``. +- Prefer using string literals. In particular, use empty string literals, + i.e. ``u""_ns`` or ``""_ns``, instead of ``Empty[C]String()`` or + ``const nsAuto[C]String empty;``. Use ``Empty[C]String()`` only if you + specifically need a ``const ns[C]String&``, e.g. with the ternary operator + or when you need to return/bind to a reference or take the address of the + empty string. +- For 16-bit literal strings, use ``u"..."_ns`` or, if necessary + ``NS_LITERAL_STRING_FROM_CSTRING(...)`` instead of ``nsAutoString()`` + or other ways that would do a run-time conversion. + See :ref:`Avoid runtime conversion of string literals` below. +- To compare a string with a literal, use ``.EqualsLiteral("...")``. +- Use ``str.IsEmpty()`` instead of ``str.Length() == 0``. +- Use ``str.Truncate()`` instead of ``str.SetLength(0)``, + ``str.Assign(""_ns)`` or ``str.AssignLiteral("")``. +- Don't use functions from ``ctype.h`` (``isdigit()``, ``isalpha()``, + etc.) or from ``strings.h`` (``strcasecmp()``, ``strncasecmp()``). + These are locale-sensitive, which makes them inappropriate for + processing protocol text. At the same time, they are too limited to + work properly for processing natural-language text. Use the + alternatives in ``mozilla/TextUtils.h`` and in ``nsUnicharUtils.h`` + in place of ``ctype.h``. In place of ``strings.h``, prefer the + ``nsStringComparator`` facilities for comparing strings or if you + have to work with zero-terminated strings, use ``nsCRT.h`` for + ASCII-case-insensitive comparison. + + +Use the ``Auto`` form of strings for local values +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +When declaring a local, short-lived ``nsString`` class, always use +``nsAutoString`` or ``nsAutoCString``. These pre-allocate a 64-byte +buffer on the stack, and avoid fragmenting the heap. Don't do this: + +.. code-block:: cpp + + nsresult + foo() + { + nsCString bar; + .. + } + +instead: + +.. code-block:: cpp + + nsresult + foo() + { + nsAutoCString bar; + .. + } + + +Be wary of leaking values from non-XPCOM functions that return char\* or PRUnichar\* +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +It is an easy trap to return an allocated string, from an internal +helper function, and then using that function inline in your code, +without freeing the value. Consider this code: + +.. code-block:: cpp + + static char* + GetStringValue() + { + .. + return resultString.ToNewCString(); + } + + .. + WarnUser(GetStringValue()); + +In the above example, ``WarnUser`` will get the string allocated from +``resultString.ToNewCString()`` and throw away the pointer. The +resulting value is never freed. Instead, either use the string classes, +to make sure your string is automatically freed when it goes out of +scope, or make sure that your string is freed. + +Automatic cleanup: + +.. code-block:: cpp + + static void + GetStringValue(nsAWritableCString& aResult) + { + .. + aResult.Assign("resulting string"); + } + + .. + nsAutoCString warning; + GetStringValue(warning); + WarnUser(warning.get()); + +Free the string manually: + +.. code-block:: cpp + + static char* + GetStringValue() + { + .. + return resultString.ToNewCString(); + } + + .. + char* warning = GetStringValue(); + WarnUser(warning); + nsMemory::Free(warning); + + +Avoid runtime conversion of string literals +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +It is very common to need to assign the value of a literal string, such +as ``"Some String"``, into a unicode buffer. Instead of using ``nsString``'s +``AssignLiteral`` and ``AppendLiteral``, use a user-defined literal like `u"foo"_ns` +instead. On most platforms, this will force the compiler to compile in a +raw unicode string, and assign it directly. In cases where the literal is defined +via a macro that is used in both 8-bit and 16-bit ways, you can use +`NS_LITERAL_STRING_FROM_CSTRING` to do the conversion at compile time. + +Incorrect: + +.. code-block:: cpp + + nsAutoString warning; + warning.AssignLiteral("danger will robinson!"); + ... + foo->SetStringValue(warning); + ... + bar->SetUnicodeValue(warning.get()); + +Correct: + +.. code-block:: cpp + + constexpr auto warning = u"danger will robinson!"_ns; + ... + // if you'll be using the 'warning' string, you can still use it as before: + foo->SetStringValue(warning); + ... + bar->SetUnicodeValue(warning.get()); + + // alternatively, use the wide string directly: + foo->SetStringValue(u"danger will robinson!"_ns); + ... + + // if a macro is the source of a 8-bit literal and you cannot change it, use + // NS_LITERAL_STRING_FROM_CSTRING, but only if necessary. + #define MY_MACRO_LITERAL "danger will robinson!" + foo->SetStringValue(NS_LITERAL_STRING_FROM_CSTRING(MY_MACRO_LITERAL)); + + // If you need to pass to a raw const char16_t *, there's no benefit to + // go through our string classes at all, just do... + bar->SetUnicodeValue(u"danger will robinson!"); + + // .. or, again, if a macro is the source of a 8-bit literal + bar->SetUnicodeValue(u"" MY_MACRO_LITERAL); + + +Usage of PR_(MAX|MIN|ABS|ROUNDUP) macro calls +--------------------------------------------- + +Use the standard-library functions (``std::max``), instead of +``PR_(MAX|MIN|ABS|ROUNDUP)``. + +Use ``mozilla::Abs`` instead of ``PR_ABS``. All ``PR_ABS`` calls in C++ code have +been replaced with ``mozilla::Abs`` calls, in `bug +847480 `__. All new +code in ``Firefox/core/toolkit`` needs to ``#include "nsAlgorithm.h"`` and +use the ``NS_foo`` variants instead of ``PR_foo``, or +``#include "mozilla/MathAlgorithms.h"`` for ``mozilla::Abs``. diff --git a/docs/code-quality/coding-style/coding_style_general.rst b/docs/code-quality/coding-style/coding_style_general.rst new file mode 100644 index 0000000000..950cd6ccd3 --- /dev/null +++ b/docs/code-quality/coding-style/coding_style_general.rst @@ -0,0 +1,18 @@ + +Mode line +~~~~~~~~~ + +Files should have Emacs and vim mode line comments as the first two +lines of the file, which should set ``indent-tabs-mode`` to ``nil``. For new +files, use the following, specifying two-space indentation: + +.. code-block:: cpp + + /* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */ + /* vim: set ts=2 et sw=2 tw=80: */ + /* This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at https://mozilla.org/MPL/2.0/. */ + +Be sure to use the correct ``Mode`` in the first line, don't use ``C++`` in +JavaScript files. diff --git a/docs/code-quality/coding-style/coding_style_java.rst b/docs/code-quality/coding-style/coding_style_java.rst new file mode 100644 index 0000000000..f2206d8e2d --- /dev/null +++ b/docs/code-quality/coding-style/coding_style_java.rst @@ -0,0 +1,68 @@ +================= +Java Coding style +================= + +- We use the `Java Coding + Style `__. + Quick summary: + + - FirstLetterUpperCase for class names. + - camelCase for method and variable names. + - One declaration per line: + + .. code-block:: java + + int x, y; // this is BAD! + int a; // split it over + int b; // two lines + +- Braces should be placed like so (generally, opening braces on same + line, closing braces on a new line): + + .. code-block:: java + + public void func(int arg) { + if (arg != 0) { + while (arg > 0) { + arg--; + } + } else { + arg++; + } + } + +- Places we differ from the Java coding style: + + - Start class variable names with 'm' prefix (e.g. + mSomeClassVariable) and static variables with 's' prefix (e.g. + sSomeStaticVariable) + - ``import`` statements: + + - Do not use wildcard imports like \`import java.util.*;\` + - Organize imports by blocks separated by empty line: + org.mozilla.*, android.*, com.*, net.*, org.*, then java.\* + This is basically what Android Studio does by default, except + that we place org.mozilla.\* at the front - please adjust + Settings -> Editor -> Code Style -> Java -> Imports + accordingly. + - Within each import block, alphabetize import names with + uppercase before lowercase. For example, ``com.example.Foo`` is + before ``com.example.bar`` + + - 4-space indents. + - Spaces, not tabs. + - Don't restrict yourself to 80-character lines. Google's Android + style guide suggests 100-character lines, which is also the + default setting in Android Studio. Java code tends to be long + horizontally, so use appropriate judgement when wrapping. Avoid + deep indents on wrapping. Note that aligning the wrapped part of a + line, with some previous part of the line (rather than just using + a fixed indent), may require shifting the code every time the line + changes, resulting in spurious whitespace changes. + +- For additional specifics on Firefox for Android, see the `Coding + Style guide for Firefox on + Android `__. +- The `Android Coding + Style `__ has some + useful guidelines too. diff --git a/docs/code-quality/coding-style/coding_style_js.rst b/docs/code-quality/coding-style/coding_style_js.rst new file mode 100644 index 0000000000..46ec1dd8a6 --- /dev/null +++ b/docs/code-quality/coding-style/coding_style_js.rst @@ -0,0 +1,150 @@ +======================= +JavaScript Coding style +======================= + +Coding style +~~~~~~~~~~~~ + +`prettier `_ is the tool used to reformat the JavaScript code. + + +Methods and functions +~~~~~~~~~~~~~~~~~~~~~ + +In JavaScript, functions should use camelCase, but should not capitalize +the first letter. Methods should not use the named function expression +syntax, because our tools understand method names: + +.. code-block:: cpp + + doSomething: function (aFoo, aBar) { + ... + } + +In-line functions should have spaces around braces, except before commas +or semicolons: + +.. code-block:: cpp + + function valueObject(aValue) { return { value: aValue }; } + + +JavaScript objects +~~~~~~~~~~~~~~~~~~ + +.. code-block:: cpp + + var foo = { prop1: "value1" }; + + var bar = { + prop1: "value1", + prop2: "value2" + }; + +Constructors for objects should be capitalized and use Pascal Case: + +.. code-block:: cpp + + function ObjectConstructor() { + this.foo = "bar"; + } + + +Operators +~~~~~~~~~ + +In JavaScript, overlong expressions not joined by ``&&`` and +``||`` should break so the operator starts on the second line and +starting in the same column as the beginning of the expression in the +first line. This applies to ``?:``, binary arithmetic operators +including ``+``, and member-of operators. Rationale: an operator at the +front of the continuation line makes for faster visual scanning, as +there is no need to read to the end of line. Also there exists a +context-sensitive keyword hazard in JavaScript; see {{bug(442099, "bug", +19)}}, which can be avoided by putting . at the start of a continuation +line, in long member expression. + +In JavaScript, ``==`` is preferred to ``===``. + +Unary keyword operators, such as ``typeof``, should have their operand +parenthesized; e.g. ``typeof("foo") == "string"``. + +Literals +~~~~~~~~ + +Double-quoted strings (e.g. ``"foo"``) are preferred to single-quoted +strings (e.g. ``'foo'``), in JavaScript, except to avoid escaping +embedded double quotes, or when assigning inline event handlers. + + +Prefixes +~~~~~~~~ + +- k=constant (e.g. ``kNC_child``). Not all code uses this style; some + uses ``ALL_CAPS`` for constants. +- g=global (e.g. ``gPrefService``) +- a=argument (e.g. ``aCount``) + +- JavaScript Specific Prefixes + + - \_=member (variable or function) (e.g. ``_length`` or + ``_setType(aType)``) + - k=enumeration value (e.g. ``const kDisplayModeNormal = 0``) + - on=event handler (e.g. ``function onLoad()``) + - Convenience constants for interface names should be prefixed with + ``nsI``: + + .. code-block:: javascript + + const nsISupports = Components.interfaces.nsISupports; + const nsIWBN = Components.interfaces.nsIWebBrowserNavigation; + + + +Other advices +~~~~~~~~~~~~~ + +- Make sure you are aware of the `JavaScript + Tips `__. +- Do not compare ``x == true`` or ``x == false``. Use ``(x)`` or + ``(!x)`` instead. ``x == true``, is certainly different from if + ``(x)``! Compare objects to ``null``, numbers to ``0`` or strings to + ``""``, if there is chance for confusion. +- Make sure that your code doesn't generate any strict JavaScript + warnings, such as: + + - Duplicate variable declaration. + - Mixing ``return;`` with ``return value;`` + - Undeclared variables or members. If you are unsure if an array + value exists, compare the index to the array's length. If you are + unsure if an object member exists, use ``"name"`` in ``aObject``, + or if you are expecting a particular type you may use + ``typeof(aObject.name) == "function"`` (or whichever type you are + expecting). + +- Use ``['value1, value2']`` to create a JavaScript array in preference + to using + ``new {{JSxRef("Array", "Array", "Syntax", 1)}}(value1, value2)`` + which can be confusing, as ``new Array(length)`` will actually create + a physically empty array with the given logical length, while + ``[value]`` will always create a 1-element array. You cannot actually + guarantee to be able to preallocate memory for an array. +- Use ``{ member: value, ... }`` to create a JavaScript object; a + useful advantage over ``new {{JSxRef("Object", "Object", "", 1)}}()`` + is the ability to create initial properties and use extended + JavaScript syntax, to define getters and setters. +- If having defined a constructor you need to assign default + properties, it is preferred to assign an object literal to the + prototype property. +- Use regular expressions, but use wisely. For instance, to check that + ``aString`` is not completely whitespace use + ``/\S/.{{JSxRef("RegExp.test", "test(aString)", "", 1)}}``. Only use + {{JSxRef("String.search", "aString.search()")}} if you need to know + the position of the result, or {{JSxRef("String.match", + "aString.match()")}} if you need to collect matching substrings + (delimited by parentheses in the regular expression). Regular + expressions are less useful if the match is unknown in advance, or to + extract substrings in known positions in the string. For instance, + {{JSxRef("String.slice", "aString.slice(-1)")}} returns the last + letter in ``aString``, or the empty string if ``aString`` is empty. + diff --git a/docs/code-quality/coding-style/coding_style_other.rst b/docs/code-quality/coding-style/coding_style_other.rst new file mode 100644 index 0000000000..2c82cc4a76 --- /dev/null +++ b/docs/code-quality/coding-style/coding_style_other.rst @@ -0,0 +1,11 @@ +================== +Other coding style +================== + +SVG practices +------------- + +Check `SVG +Guidelines `__ for +more details. + diff --git a/docs/code-quality/coding-style/coding_style_python.rst b/docs/code-quality/coding-style/coding_style_python.rst new file mode 100644 index 0000000000..3a818fcfd4 --- /dev/null +++ b/docs/code-quality/coding-style/coding_style_python.rst @@ -0,0 +1,71 @@ +=================== +Python Coding style +=================== + +Coding style +~~~~~~~~~~~~ + + :ref:`black` is the tool used to reformat the Python code. + +Linting +~~~~~~~ + +The Python linting is done by :ref:`Flake8` and :ref:`pylint` +They are executed by mozlint both at review phase and in the CI. + +Indentation +~~~~~~~~~~~ + +Four spaces in Python code. + + +Makefile/moz.build practices +---------------------------- + +- Changes to makefile and moz.build variables do not require + build-config peer review. Any other build system changes, such as + adding new scripts or rules, require review from the build-config + team. +- Suffix long ``if``/``endif`` conditionals with #{ & #}, so editors + can display matched tokens enclosing a block of statements. + + :: + + ifdef CHECK_TYPE #{ + ifneq ($(flavor var_type),recursive) #{ + $(warning var should be expandable but detected var_type=$(flavor var_type)) + endif #} + endif #} + +- moz.build are python and follow normal Python style. +- List assignments should be written with one element per line. Align + closing square brace with start of variable assignment. If ordering + is not important, variables should be in alphabetical order. + + .. code-block:: python + + var += [ + 'foo', + 'bar' + ] + +- Use ``CONFIG['CPU_ARCH'] {=arm}`` to test for generic classes of + architecture rather than ``CONFIG['OS_TEST'] {=armv7}`` (re: bug 886689). + + +Other advices +~~~~~~~~~~~~~ + +- Install the + `mozext `__ + Mercurial extension, and address every issue reported on commit + or the output of ``hg critic``. +- Follow `PEP 8 `__. Please run :ref:`black` for this. +- Do not place statements on the same line as ``if/elif/else`` + conditionals to form a one-liner. +- Global vars, please avoid them at all cost. +- Exclude outer parenthesis from conditionals.Use + ``if x > 5:,``\ rather than ``if (x > 5):`` +- Use string formatters, rather than var + str(val). + ``var = 'Type %s value is %d'% ('int', 5).`` +- Testing/Unit tests, please write them and make sure that they are executed in the CI. diff --git a/docs/code-quality/coding-style/format_cpp_code_with_clang-format.rst b/docs/code-quality/coding-style/format_cpp_code_with_clang-format.rst new file mode 100644 index 0000000000..10d2bc00fc --- /dev/null +++ b/docs/code-quality/coding-style/format_cpp_code_with_clang-format.rst @@ -0,0 +1,272 @@ +===================================== +Formatting C++ Code With clang-format +===================================== + +Mozilla uses the Google coding style for whitespace, which is enforced +using `clang-format `__. A +specific version of the binary will be installed when +``./mach clang-format`` or ``./mach bootstrap`` are run. We build our +own binaries and update them as needed. + +Options are explicitly defined `in clang-format +itself `__. +If the options are changed in clang upstream, this might cause some +changes in the Firefox tree. For this reason, it is best to use the +mozilla-provided binaries. + +Manual formatting +----------------- + +We provide a mach subcommand for running clang-format from the +command-line. This wrapper handles ensuring the correct version of +clang-format is installed and run. + +If clang-format isn’t installed, the binaries will be automatically +downloaded from taskcluster and installed into ~/.mozbuild. We build our +own clang-format binaries. + + +Formatting local changes +~~~~~~~~~~~~~~~~~~~~~~~~ + +:: + + $ ./mach clang-format + +When run without arguments, it will run on a local diff. This could miss +some reformatting (for example, when blocks are touched). +(`searchfox `__) + + +Formatting specific paths +~~~~~~~~~~~~~~~~~~~~~~~~~ + +:: + + $ ./mach clang-format -p # Format in-place + $ ./mach clang-format -p -s # Show changes + +The command also accepts a ``-p`` argument to reformat a specific +directory or file, and a ``-s`` flag to show the changes instead of +applying them to the working directory +(`searchfox `__) + + +Formatting specific commits / revisions +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +:: + + $ ./mach clang-format -c HEAD # Format a single git commit + $ ./mach clang-format -c HEAD~~..HEAD # Format a range of git commits + $ ./mach clang-format -c . # Format a single mercurial revision + +The command accepts a ``-c`` argument that takes a revision number or +commit range, and will format the lines modified by those commits. +(`searchfox `__) + + +Scripting Clang-Format +~~~~~~~~~~~~~~~~~~~~~~ + +Clang format expects that the path being passed to it is the path +on-disk. If this is not the case, for example when formatting a +temporary file, the "real" path must be specified. This can be done with +the ``--assume-filename `` argument. + + +Configuring the clang-format commit hook +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +To run clang-format at commit phase, run ``mach boostrap`` or just add +the following line in the ``hgrc`` file: + +.. code:: ini + + [extensions] + clang-format = ~/.mozbuild/version-control-tools/hgext/clang-format + +We use a hg extension as they are more flexible than hooks. + +With git, the configuration is the following: + +:: + + # From the root git directory: + $ ln -s $(pwd)/tools/lint/hooks_clang_format.py .git/hooks/pre-commit + +You'll likely need to install the ``python-hglib`` package for your OS, +or else you may get errors like ``abort: No module named hglib.client!`` +when you try to commit. + + +Editor integration +------------------ + +It is possible to configure many editors to support running +``clang-format`` automatically on save, or when run from within the +editor. + + +Editor plugins +~~~~~~~~~~~~~~ + +- `Atom `__ +- `BBEdit `__ + + - `clang-format-bbedit.applescript `__ + +- Eclipse + + - Install the + `CppStyle `__ + plugin + - In Preferences -> C/C++ -> CppStyle, set the clang-format path to + ~/.mozbuild/clang-tools/clang-tidy/bin/clang-format + - (Optional) check "Run clang-format on file save" + +- `Emacs `__ + + - `clang-format.el `__ + (Or install + `clang-format `__ from MELPA) + - `google-c-style `__ from MELPA + +- `Sublime Text `__ + + - `alternative + tool `__ + +- `Vim `__ + + - `clang-format.py `__ + - `vim-clang-format `__ + +- `Visual + Studio `__ + + - `llvm.org plugin `__ + - `Integrated support in Visual Studio + 2017 `__ + +- `Visual Studio + Code `__ +- `XCode `__ +- `Script for patch + reformatting `__ + + - `clang-format-diff.py `__ + + +Configuration +~~~~~~~~~~~~~ + +These tools generally run clang-format themselves, and won't use +``./mach clang-format``. The binary installed by our tooling will be +located at ``~/.mozbuild/clang-tools/clang-tidy/bin/clang-format``. + +You typically shouldn't need to specify any other special configuration +in your editor besides the clang-format binary. Most of the +configuration that clang-format relies on for formatting is stored +inside our source tree. More specifically, using the .clang-format file +located in the root of the repository. Please note that this doesn't +include the list of ignored files and directories (provided by +.clang-format-ignore which is a feature provided by the mach command +wrapper). + +Coding style configuration is done within clang-format itself. When we +change the configuration (incorrect configuration, new feature in clang, +etc), we use `local +overrides `__. + + +Ignored files & directories +~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +We maintain a `list of ignored directories and +files `__, +which is used by ``./mach clang-format``. This is generally only used +for code broken by clang-format, and third-party code. + + +Ignored code hunks +~~~~~~~~~~~~~~~~~~ + +Sections of code may have formatting disabled using comments. If a +section must not be formatted, the following comments will disable the +reformat: + +:: + + // clang-format off + my code which should not be reformated + // clang-format on + +You can find an `example of code not +formatted `__. + + +Merging formatted and unformatted code +-------------------------------------- + +During the transition to using chromium style enforced by clang-format +for all code in tree, it will often be necessary to rebase non-formatted +code onto a formatted tree. + + +Mercurial +~~~~~~~~~ + +The ``format-source`` extension, now bundled with +``version-control-tools``, and installed by ``./mach bootstrap``, may be +used to seamlessly handle this situation. More details may be found in +this +`document `__. + +The parent changeset of the reformat has been tagged as +``PRE_TREEWIDE_CLANG_FORMAT``. + + +Git +~~~ + +To perform a rebase onto mozilla-central after the merge, a handy merge +driver, ``clang-format-merge``, has been written: + +.. code:: shell + + $ git clone https://github.com/emilio/clang-format-merge + $ /path/to/clang-format-merge/git-wrapper rebase + +The wrapper should clean up after itself, and the clone may be deleted +after the rebase is complete. + + +Ignore lists +------------ + +To make sure that the blame/annotate features of Mercurial or git aren't +affected. Two files are maintained to keep track of the reformatting +commits. + + +With Mercurial +~~~~~~~~~~~~~~ + +| The list is stored in + `https://searchfox.org/mozilla-central/source/.hg-annotate-ignore-revs `__ +| Commit messages should also contain the string ``# ignore-this-changeset`` + +The syntax in this file is generated using the following syntax: + +:: + + $ hg log --template '{node} - {author|person} - {desc|strip|firstline}\n' + +With git +~~~~~~~~ + +The list is stored in +`https://searchfox.org/mozilla-central/source/.git-blame-ignore-revs `__ +and contains git revisions for both gecko-dev and the git cinnabar +repository. diff --git a/docs/code-quality/coding-style/index.rst b/docs/code-quality/coding-style/index.rst new file mode 100644 index 0000000000..e62ce910ca --- /dev/null +++ b/docs/code-quality/coding-style/index.rst @@ -0,0 +1,20 @@ +Coding style +============ + +Firefox code is using different programming languages. +For each language, we are enforcing a specific coding style. + +Getting Help +------------ + +If you need help or have questions, please don’t hesitate to contact us via Matrix +in the "Lint and Formatting" room +(`#lint:mozilla.org `_). + + +.. toctree:: + :caption: Coding Style User Guide + :maxdepth: 2 + :glob: + + * diff --git a/docs/code-quality/coding-style/using_cxx_in_firefox_code.rst b/docs/code-quality/coding-style/using_cxx_in_firefox_code.rst new file mode 100644 index 0000000000..513098855a --- /dev/null +++ b/docs/code-quality/coding-style/using_cxx_in_firefox_code.rst @@ -0,0 +1,1065 @@ +Using C++ in Mozilla code +========================= + +C++ language features +--------------------- + +Mozilla code only uses a subset of C++. Runtime type information (RTTI) +is disabled, as it tends to cause a very large increase in codesize. +This means that ``dynamic_cast``, ``typeid()`` and ```` cannot +be used in Mozilla code. Also disabled are exceptions; do not use +``try``/``catch`` or throw any exceptions. Libraries that throw +exceptions may be used if you are willing to have the throw instead be +treated as an abort. + +On the side of extending C++, we compile with ``-fno-strict-aliasing``. +This means that when reinterpreting a pointer as a differently-typed +pointer, you don't need to adhere to the "effective type" (of the +pointee) rule from the standard (aka. "the strict aliasing rule") when +dereferencing the reinterpreted pointer. You still need make sure that +you don't violate alignment requirements and need to make sure that the +data at the memory location pointed to forms a valid value when +interpreted according to the type of the pointer when dereferencing the +pointer for reading. Likewise, if you write by dereferencing the +reinterpreted pointer and the originally-typed pointer might still be +dereferenced for reading, you need to make sure that the values you +write are valid according to the original type. This value validity +issue is moot for e.g. primitive integers for which all bit patterns of +their size are valid values. + +- As of Mozilla 59, C++14 mode is required to build Mozilla. +- As of Mozilla 67, MSVC can no longer be used to build Mozilla. +- As of Mozilla 73, C++17 mode is required to build Mozilla. + +This means that C++17 can be used where supported on all platforms. The +list of acceptable features is given below: + +.. list-table:: + :widths: 25 25 25 25 + :header-rows: 3 + + * - + - GCC + - Clang + - + * - Current minimal requirement + - 7.1 + - 5.0 + - + * - Feature + - GCC + - Clang + - Can be used in code + * - ``type_t &&`` + - 4.3 + - 2.9 + - Yes (see notes) + * - ref qualifiers on methods + - 4.8.1 + - 2.9 + - Yes + * - default member - initializers (except for bit-fields) + - 4.7 + - 3.0 + - Yes + * - default member - initializers (for bit-fields) + - 8 + - 6 + - **No** + * - variadic templates + - 4.3 + - 2.9 + - Yes + * - Initializer lists + - 4.4 + - 3.1 + - Yes + * - ``static_assert`` + - 4.3 + - 2.9 + - Yes + * - ``auto`` + - 4.4 + - 2.9 + - Yes + * - lambdas + - 4.5 + - 3.1 + - Yes + * - ``decltype`` + - 4.3 + - 2.9 + - Yes + * - ``Foo>`` + - 4.3 + - 2.9 + - Yes + * - ``auto func() -> int`` + - 4.4 + - 3.1 + - Yes + * - Templated aliasing + - 4.7 + - 3.0 + - Yes + * - ``nullptr`` + - 4.6 + - 3.0 + - Yes + * - ``enum foo : int16_t`` {}; + - 4.4 + - 2.9 + - Yes + * - ``enum class foo {}``; + - 4.4 + - 2.9 + - Yes + * - ``enum foo;`` + - 4.6 + - 3.1 + - Yes + * - ``[[attributes]]`` + - 4.8 + - 3.3 + - **No** (see notes) + * - ``constexpr`` + - 4.6 + - 3.1 + - Yes + * - ``alignas`` + - 4.8 + - 3.3 + - Yes + * - ``alignof`` + - 4.8 + - 3.3 + - Yes, but see notes ; only clang 3.6 claims as_feature(cxx_alignof) + * - Delegated constructors + - 4.7 + - 3.0 + - Yes + * - Inherited constructors + - 4.8 + - 3.3 + - Yes + * - ``explicit operator bool()`` + - 4.5 + - 3.0 + - Yes + * - ``char16_t/u"string"`` + - 4.4 + - 3.0 + - Yes + * - ``R"(string)"`` + - 4.5 + - 3.0 + - Yes + * - ``operator""()`` + - 4.7 + - 3.1 + - Yes + * - ``=delete`` + - 4.4 + - 2.9 + - Yes + * - ``=default`` + - 4.4 + - 3.0 + - Yes + * - unrestricted unions + - 4.6 + - 3.1 + - Yes + * - ``for (auto x : vec)`` (`be careful about the type of the iterator `__) + - 4.6 + - 3.0 + - Yes + * - ``override``/``final`` + - 4.7 + - 3.0 + - Yes + * - ``thread_local`` + - 4.8 + - 3.3 + - **No** (see notes) + * - function template default arguments + - 4.3 + - 2.9 + - Yes + * - local structs as template parameters + - 4.5 + - 2.9 + - Yes + * - extended friend declarations + - 4.7 + - 2.9 + - Yes + * - ``0b100`` (C++14) + - 4.9 + - 2.9 + - Yes + * - `Tweaks to some C++ contextual conversions` (C++14) + - 4.9 + - 3.4 + - Yes + * - Return type deduction (C++14) + - 4.9 + - 3.4 + - Yes (but only in template code when you would have used ``decltype (complex-expression)``) + * - Generic lambdas (C++14) + - 4.9 + - 3.4 + - Yes + * - Initialized lambda captures (C++14) + - 4.9 + - 3.4 + - Yes + * - Digit separator (C++14) + - 4.9 + - 3.4 + - Yes + * - Variable templates (C++14) + - 5.0 + - 3.4 + - Yes + * - Relaxed constexpr (C++14) + - 5.0 + - 3.4 + - Yes + * - Aggregate member initialization (C++14) + - 5.0 + - 3.3 + - Yes + * - Clarifying memory allocation (C++14) + - 5.0 + - 3.4 + - Yes + * - [[deprecated]] attribute (C++14) + - 4.9 + - 3.4 + - **No** (see notes) + * - Sized deallocation (C++14) + - 5.0 + - 3.4 + - **No** (see notes) + * - Concepts (Concepts TS) + - 6.0 + - — + - **No** + * - Inline variables (C++17) + - 7.0 + - 3.9 + - **No** (clang 5 has bugs with inline variables) + * - constexpr_if (C++17) + - 7.0 + - 3.9 + - Yes + * - constexpr lambdas (C++17) + - — + - — + - **No** + * - Structured bindings (C++17) + - 7.0 + - 4.0 + - Yes + * - Separated declaration and condition in ``if``, ``switch`` (C++17) + - 7.0 + - 3.9 + - Yes + * - `Fold expressions `__ (C++17) + - 6.0 + - 3.9 + - Yes + * - [[fallthrough]], [[maybe_unused]], [[nodiscard]] (C++17) + - 7.0 + - 3.9 + - Yes + * - Aligned allocation/deallocation (C++17) + - 7.0 + - 4.0 + - **No** (see notes) + * - #pragma once + - 3.4 + - Yes + - **Not** until we `normalize headers `__ + * - `Source code information capture `__ + - 8.0 + - — + - **No** + +Sources +~~~~~~~ + +* GCC: https://gcc.gnu.org/projects/cxx-status.html +* Clang: https://clang.llvm.org/cxx_status.html + +Notes +~~~~~ + +rvalue references: Implicit move method generation cannot be used. + +Attributes: Several common attributes are defined in +`mozilla/Attributes.h `__ +or nscore.h. + +Alignment: Some alignment utilities are defined in +`mozilla/Alignment.h `__. +/!\\ MOZ_ALIGNOF and alignof don't have the same semantics. Be careful +of what you expect from them. + +``[[deprecated]]``: If we have deprecated code, we should be removing it +rather than marking it as such. Marking things as ``[[deprecated]]`` +also means the compiler will warn if you use the deprecated API, which +turns into a fatal error in our automation builds, which is not helpful. + +Sized deallocation: Our compilers all support this (custom flags are +required for GCC and Clang), but turning it on breaks some classes' +``operator new`` methods, and `some +work `__ would +need to be done to make it an efficiency win with our custom memory +allocator. + +Aligned allocation/deallocation: Our custom memory allocator doesn't +have support for these functions. + +Thread locals: ``thread_local`` is not supported on Android. + + +C++ and Mozilla standard libraries +---------------------------------- + +The Mozilla codebase contains within it several subprojects which follow +different rules for which libraries can and can't be used it. The rules +listed here apply to normal platform code, and assume unrestricted +usability of MFBT or XPCOM APIs. + +.. warning:: + + The rest of this section is a draft for expository and exploratory + purposes. Do not trust the information listed here. + +What follows is a list of standard library components provided by +Mozilla or the C++ standard. If an API is not listed here, then it is +not permissible to use it in Mozilla code. Deprecated APIs are not +listed here. In general, prefer Mozilla variants of data structures to +standard C++ ones, even when permitted to use the latter, since Mozilla +variants tend to have features not found in the standard library (e.g., +memory size tracking) or have more controllable performance +characteristics. + +A list of approved standard library headers is maintained in +`config/stl-headers.mozbuild `__. + + +Data structures +~~~~~~~~~~~~~~~ + +.. list-table:: + :widths: 25 25 25 25 + :header-rows: 1 + + * - Name + - Header + - STL equivalent + - Notes + * - ``nsAutoTArray`` + - ``nsTArray.h`` + - + - Like ``nsTArray``, but will store a small amount as stack storage + * - ``nsAutoTObserverArray`` + - ``nsTObserverArray.h`` + - + - Like ``nsTObserverArray``, but will store a small amount as stack storage + * - ``mozilla::BloomFilter`` + - ``mozilla/BloomFilter.h`` + - + - Probabilistic set membership (see `Wikipedia `__) + * - ``nsClassHashtable`` + - ``nsClassHashtable.h`` + - + - Adaptation of nsTHashtable, see `XPCOM hashtable guide `__ + * - ``nsCOMArray`` + - ``nsCOMArray.h`` + - + - Like ``nsTArray>`` + * - ``nsDataHashtable`` + - ``nsClassHashtable.h`` + - ``std::unordered_map`` + - Adaptation of ``nsTHashtable``, see `XPCOM hashtable guide `__ + * - ``nsDeque`` + - ``nsDeque.h`` + - ``std::deque`` + - + * - ``mozilla::EnumSet`` + - ``mozilla/EnumSet.h`` + - + - Like ``std::set``, but for enum classes. + * - ``mozilla::Hash{Map,Set}`` + - `mozilla/HashTable.h `__ + - ``std::unordered_{map,set}`` + - A general purpose hash map and hash set. + * - ``nsInterfaceHashtable`` + - ``nsInterfaceHashtable.h`` + - ``std::unordered_map`` + - Adaptation of ``nsTHashtable``, see `XPCOM hashtable guide `__ + * - ``nsJSThingHashtable`` + - ``nsJSThingHashtable.h`` + - + - Adaptation of ``nsTHashtable``, see `XPCOM hashtable guide `__ + * - ``mozilla::LinkedList`` + - ``mozilla/LinkedList.h`` + - ``std::list`` + - Doubly-linked list + * - ``nsRef PtrHashtable`` + - ``nsRefPtrHashtable.h`` + - ``std::unordered_map`` + - Adaptation of ``nsTHashtable``, see `XPCOM hashtable guide `__ + * - ``mozilla::SegmentedVector`` + - ``mozilla/SegmentedVector.h`` + - ``std::deque`` w/o O(1) pop_front + - Doubly-linked list of vector elements + * - ``mozilla::SplayTree`` + - ``mozilla/SplayTree.h`` + - + - Quick access to recently-accessed elements (see `Wikipedia `__) + * - ``nsTArray`` + - ``nsTArray.h`` + - ``std::vector`` + - + * - ``nsTHashtable`` + - ``nsTHashtable.h`` + - ``std::unordered_{map,set}`` + - See `XPCOM hashtable guide `__, you probably want a subclass + * - ``nsTObserverArray`` + - ``nsTObserverArray.h`` + - + - Like ``nsTArray``, but iteration is stable even through mutation + * - ``nsTPriorityQueue`` + - ``nsTPriorityQueue.h`` + - ``std::priority_queue`` + - Unlike the STL class, not a container adapter + * - ``mozilla::Vector`` + - ``mozilla/Vector.h`` + - ``std::vector`` + - + * - ``mozilla::Buffer`` + - ``mozilla/Buffer.h`` + - + - Unlike ``Array``, has a run-time variable length. Unlike ``Vector``, does not have capacity and growth mechanism. Unlike ``Span``, owns its buffer. + + +Safety utilities +~~~~~~~~~~~~~~~~ + +.. list-table:: + :widths: 25 25 25 25 + :header-rows: 1 + + * - Name + - Header + - STL equivalent + - Notes + * - ``mozilla::Array`` + - ``mfbt/Array.h`` + - + - safe array index + * - ``mozilla::AssertedCast`` + - ``mfbt/Casting.h`` + - + - casts + * - ``mozilla::CheckedInt`` + - ``mfbt/CheckedInt.h`` + - + - avoids overflow + * - ``nsCOMPtr`` + - ``xpcom/base/nsCOMPtr.h`` + - ``std::shared_ptr`` + - + * - ``mozilla::EnumeratedArray`` + - ``mfbt/EnumeratedArray.h`` + - ``mozilla::Array`` + - + * - ``mozilla::Maybe`` + - ``mfbt/Maybe.h`` + - ``std::optional`` + - + * - ``mozilla::RangedPtr`` + - ``mfbt/RangedPtr.h`` + - + - like ``mozilla::Span`` but with two pointers instead of pointer and length + * - ``mozilla::RefPtr`` + - ``mfbt/RefPtr.h`` + - ``std::shared_ptr`` + - + * - ``mozilla::Span`` + - ``mozilla/Span.h`` + - ``gsl::span``, ``absl::Span``, ``std::string_view``, ``std::u16string_view`` + - Rust's slice concept for C++ (without borrow checking) + * - ``StaticRefPtr`` + - ``xpcom/base/StaticPtr.h`` + - + - ``nsRefPtr`` w/o static constructor + * - ``mozilla::UniquePtr`` + - ``mfbt/UniquePtr.h`` + - ``std::unique_ptr`` + - + * - ``mozilla::WeakPtr`` + - ``mfbt/WeakPtr.h`` + - ``std::weak_ptr`` + - + * - ``nsWeakPtr`` + - ``xpcom/base/nsWeakPtr.h`` + - ``std::weak_ptr`` + - + + +Strings +~~~~~~~ + +See the `Mozilla internal string +guide `__ for +usage of ``nsAString`` (our copy-on-write replacement for +``std::u16string``) and ``nsACString`` (our copy-on-write replacement +for ``std::string``). + +Be sure not to introduce further uses of ``std::wstring``, which is not +portable! (Some uses exist in the IPC code.) + + +Algorithms +~~~~~~~~~~ + +.. list-table:: + :widths: 25 25 + + * - ``mozilla::BinarySearch`` + - ``mfbt/BinarySearch.h`` + * - ``mozilla::BitwiseCast`` + - ``mfbt/Casting.h`` (strict aliasing-safe cast) + * - ``mozilla/MathAlgorithms.h`` + - (rotate, ctlz, popcount, gcd, abs, lcm) + * - ``mozilla::RollingMean`` + - ``mfbt/RollingMean.h`` () + + +Concurrency +~~~~~~~~~~~ + +.. list-table:: + :widths: 25 25 25 25 + :header-rows: 1 + + * - Name + - Header + - STL/boost equivalent + - Notes + * - ``mozilla::Atomic`` + - mfbt/Atomic.h + - ``std::atomic`` + - + * - ``mozilla::CondVar`` + - xpcom/threads/CondVar.h + - ``std::condition_variable`` + - + * - ``mozilla::DataMutex`` + - xpcom/threads/DataMutex.h + - ``boost::synchronized_value`` + - + * - ``mozilla::Monitor`` + - xpcom/threads/Monitor.h + - + - + * - ``mozilla::Mutex`` + - xpcom/threads/Mutex.h + - ``std::mutex`` + - + * - ``mozilla::ReentrantMonitor`` + - xpcom/threads/ReentrantMonitor.h + - + - + * - ``mozilla::StaticMutex`` + - xpcom/base/StaticMutex.h + - ``std::mutex`` + - Mutex that can (and in fact, must) be used as a global/static variable. + + +Miscellaneous +~~~~~~~~~~~~~ + +.. list-table:: + :widths: 25 25 25 25 + :header-rows: 1 + + * - Name + - Header + - STL/boost equivalent + - Notes + * - ``mozilla::AlignedStorage`` + - mfbt/Alignment.h + - ``std::aligned_storage`` + - + * - ``mozilla::MaybeOneOf`` + - mfbt/MaybeOneOf.h + - ``std::optional>`` + - ~ ``mozilla::Maybe`` + * - ``mozilla::Pair`` + - mfbt/Pair.h + - ``std::tuple`` + - minimal space! + * - ``mozilla::TimeStamp`` + - xpcom/ds/TimeStamp.h + - ``std::chrono::time_point`` + - + * - + - mozilla/TypeTraits.h + - ```` + - + * - + - mozilla/PodOperations.h + - + - C++ versions of ``memset``, ``memcpy``, etc. + * - + - mozilla/ArrayUtils.h + - + - + * - + - mozilla/Compression.h + - + - + * - + - mozilla/Endian.h + - + - + * - + - mozilla/FloatingPoint.h + - + - + * - + - mozilla/HashFunctions.h + - ``std::hash`` + - + * - + - mozilla/Move.h + - ``std::move``, ``std::swap``, ``std::forward`` + - + + +Mozilla data structures and standard C++ ranges and iterators +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +Some Mozilla-defined data structures provide STL-style +`iterators `__ and +are usable in `range-based for +loops `__ as well +as STL `algorithms `__. + +Currently, these include: + +.. list-table:: + :widths: 16 16 16 16 16 + :header-rows: 1 + + * - Name + - Header + - Bug(s) + - Iterator category + - Notes + * - ``nsTArray`` + - ``xpcom/ds/n sTArray.h`` + - `1126552 `__ + - Random-access + - Also reverse-iterable. Also supports remove-erase pattern via RemoveElementsAt method. Also supports back-inserting output iterators via ``MakeBackInserter`` function. + * - ``nsBaseHashtable`` and subclasses: ``nsClassHashtable`` ``nsDataHashtable`` ``nsInterfaceHashtable`` ``nsJSThingHashtable`` ``nsRefPtrHashtable`` + - ``xpcom/ds/nsBaseHashtable.h`` ``xpcom/ds/nsClassHashtable.h`` ``xpcom/ds/nsDataHashtable.h`` ``xpcom/ds/nsInterfaceHashtable.h`` ``xpcom/ds/nsJSThingHashtable.h`` ``xpcom/ds/nsRefPtrHashtable.h`` + - `1575479 `__ + - Forward + - + * - ``nsCOMArray`` + - ``xpcom/ds/nsCOMArray.h`` + - `1342303 `__ + - Random-access + - Also reverse-iterable. + * - ``Array`` ``EnumerationArray`` ``RangedArray`` + - ``mfbt/Array.h`` ``mfbt/EnumerationArray.h`` ``mfbt/RangedArray.h`` + - `1216041 `__ + - Random-access + - Also reverse-iterable. + * - ``Buffer`` + - ``mfbt/Buffer.h`` + - `1512155 `__ + - Random-access + - Also reverse-iterable. + * - ``DoublyLinkedList`` + - ``mfbt/DoublyLinkedList.h`` + - `1277725 `__ + - Forward + - + * - ``EnumeratedRange`` + - ``mfbt/EnumeratedRange.h`` + - `1142999 `__ + - *Missing* + - Also reverse-iterable. + * - ``IntegerRange`` + - ``mfbt/IntegerRange.h`` + - `1126701 `__ + - *Missing* + - Also reverse-iterable. + * - ``SmallPointerArray`` + - ``mfbt/SmallPointerArray.h`` + - `1331718 `__ + - Random-access + - + * - ``Span`` + - ``mfbt/Span.h`` + - `1295611 `__ + - Random-access + - Also reverse-iterable. + +Note that if the iterator category is stated as "missing", the type is +probably only usable in range-based for. This is most likely just an +omission, which could be easily fixed. + +Useful in this context are also the class template ``IteratorRange`` +(which can be used to construct a range from any pair of iterators) and +function template ``Reversed`` (which can be used to reverse any range), +both defined in ``mfbt/ReverseIterator.h`` + + +Further C++ rules +----------------- + + +Don't use static constructors +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +(You probably shouldn't be using global variables to begin with. Quite +apart from the weighty software-engineering arguments against them, +globals affect startup time! But sometimes we have to do ugly things.) + +Non-portable example: + +.. code-block:: c++ + + FooBarClass static_object(87, 92); + + void + bar() + { + if (static_object.count > 15) { + ... + } + } + +Once upon a time, there were compiler bugs that could result in +constructors not being called for global objects. Those bugs are +probably long gone by now, but even with the feature working correctly, +there are so many problems with correctly ordering C++ constructors that +it's easier to just have an init function: + +.. code-block:: c++ + + static FooBarClass* static_object; + + FooBarClass* + getStaticObject() + { + if (!static_object) + static_object = + new FooBarClass(87, 92); + return static_object; + } + + void + bar() + { + if (getStaticObject()->count > 15) { + ... + } + } + + +Don't use exceptions +~~~~~~~~~~~~~~~~~~~~ + +See the introduction to the "C++ language features" section at the start +of this document. + + +Don't use Run-time Type Information +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +See the introduction to the "C++ language features" section at the start +of this document. + +If you need runtime typing, you can achieve a similar result by adding a +``classOf()`` virtual member function to the base class of your +hierarchy and overriding that member function in each subclass. If +``classOf()`` returns a unique value for each class in the hierarchy, +you'll be able to do type comparisons at runtime. + + +Don't use the C++ standard library (including iostream and locale) +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +See the section "C++ and Mozilla standard libraries". + + +Use C++ lambdas, but with care +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +C++ lambdas are supported across all our compilers now. Rejoice! We +recommend explicitly listing out the variables that you capture in the +lambda, both for documentation purposes, and to double-check that you're +only capturing what you expect to capture. + + +Use namespaces +~~~~~~~~~~~~~~ + +Namespaces may be used according to the style guidelines in :ref:`C++ Coding style`. + + +Don't mix varargs and inlines +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +What? Why are you using varargs to begin with?! Stop that at once! + + +Make header files compatible with C and C++ +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +Non-portable example: + +.. code-block:: c++ + + /*oldCheader.h*/ + int existingCfunction(char*); + int anotherExistingCfunction(char*); + + /* oldCfile.c */ + #include "oldCheader.h" + ... + + // new file.cpp + extern "C" { + #include "oldCheader.h" + }; + ... + +If you make new header files with exposed C interfaces, make the header +files work correctly when they are included by both C and C++ files. + +(If you need to include a C header in new C++ files, that should just +work. If not, it's the C header maintainer's fault, so fix the header if +you can, and if not, whatever hack you come up with will probably be +fine.) + +Portable example: + +.. code-block:: c++ + + /* oldCheader.h*/ + PR_BEGIN_EXTERN_C + int existingCfunction(char*); + int anotherExistingCfunction(char*); + PR_END_EXTERN_C + + /* oldCfile.c */ + #include "oldCheader.h" + ... + + // new file.cpp + #include "oldCheader.h" + ... + +There are number of reasons for doing this, other than just good style. +For one thing, you are making life easier for everyone else, doing the +work in one common place (the header file) instead of all the C++ files +that include it. Also, by making the C header safe for C++, you document +that "hey, this file is now being included in C++". That's a good thing. +You also avoid a big portability nightmare that is nasty to fix... + + +Use override on subclass virtual member functions +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +The ``override`` keyword is supported in C++11 and in all our supported +compilers, and it catches bugs. + + +Always declare a copy constructor and assignment operator +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +Many classes shouldn't be copied or assigned. If you're writing one of +these, the way to enforce your policy is to declare a deleted copy +constructor as private and not supply a definition. While you're at it, +do the same for the assignment operator used for assignment of objects +of the same class. Example: + +.. code-block:: c++ + + class Foo { + ... + private: + Foo(const Foo& x) = delete; + Foo& operator=(const Foo& x) = delete; + }; + +Any code that implicitly calls the copy constructor will hit a +compile-time error. That way nothing happens in the dark. When a user's +code won't compile, they'll see that they were passing by value, when +they meant to pass by reference (oops). + + +Be careful of overloaded methods with like signatures +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +It's best to avoid overloading methods when the type signature of the +methods differs only by one "abstract" type (e.g. ``PR_Int32`` or +``int32``). What you will find as you move that code to different +platforms, is suddenly on the Foo2000 compiler your overloaded methods +will have the same type-signature. + + +Type scalar constants to avoid unexpected ambiguities +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +Non-portable code: + +.. code-block:: c++ + + class FooClass { + // having such similar signatures + // is a bad idea in the first place. + void doit(long); + void doit(short); + }; + + void + B::foo(FooClass* xyz) + { + xyz->doit(45); + } + +Be sure to type your scalar constants, e.g., ``uint32_t(10)`` or +``10L``. Otherwise, you can produce ambiguous function calls which +potentially could resolve to multiple methods, particularly if you +haven't followed (2) above. Not all of the compilers will flag ambiguous +method calls. + +Portable code: + +.. code-block:: c++ + + class FooClass { + // having such similar signatures + // is a bad idea in the first place. + void doit(long); + void doit(short); + }; + + void + B::foo(FooClass* xyz) + { + xyz->doit(45L); + } + + +Use nsCOMPtr in XPCOM code +~~~~~~~~~~~~~~~~~~~~~~~~~~ + +See the ``nsCOMPtr`` `User +Manual `__ for +usage details. + + +Don't use identifiers that start with an underscore +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +This rule occasionally surprises people who've been hacking C++ for +decades. But it comes directly from the C++ standard! + +According to the C++ Standard, 17.4.3.1.2 Global Names +[lib.global.names], paragraph 1: + +Certain sets of names and function signatures are always reserved to the +implementation: + +- Each name that contains a double underscore (__) or begins with an + underscore followed by an uppercase letter (2.11) is reserved to the + implementation for any use. +- **Each name that begins with an underscore is reserved to the + implementation** for use as a name in the global namespace. + + +Stuff that is good to do for C or C++ +------------------------------------- + + +Avoid conditional #includes when possible +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +Don't write an ``#include`` inside an ``#ifdef`` if you could instead +put it outside. Unconditional includes are better because they make the +compilation more similar across all platforms and configurations, so +you're less likely to cause stupid compiler errors on someone else's +favorite platform that you never use. + +Bad code example: + +.. code-block:: c++ + + #ifdef MOZ_ENABLE_JPEG_FOUR_BILLION + #include // <--- don't do this + #include "jpeg4e9.h" // <--- only do this if the header really might not be there + #endif + +Of course when you're including different system files for different +machines, you don't have much choice. That's different. + + +Every .cpp source file should have a unique name +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +Every object file linked into libxul needs to have a unique name. Avoid +generic names like nsModule.cpp and instead use nsPlacesModule.cpp. + + +Turn on warnings for your compiler, and then write warning free code +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +What generates a warning on one platform will generate errors on +another. Turn warnings on. Write warning-free code. It's good for you. +Treat warnings as errors by adding +``ac_add_options --enable-warnings-as-errors`` to your mozconfig file. + + +Use the same type for all bitfields in a ``struct`` or ``class`` +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +Some compilers do not pack the bits when different bitfields are given +different types. For example, the following struct might have a size of +8 bytes, even though it would fit in 1: + +.. code-block:: c++ + + struct { + char ch: 1; + int i: 1; + }; + + +Don't use an enum type for a bitfield +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +The classic example of this is using ``PRBool`` for a boolean bitfield. +Don't do that. ``PRBool`` is a signed integer type, so the bitfield's +value when set will be ``-1`` instead of ``+1``, which---I know, +*crazy*, right? The things C++ hackers used to have to put up with... + +You shouldn't be using ``PRBool`` anyway. Use ``bool``. Bitfields of +type ``bool`` are fine. + +Enums are signed on some platforms (in some configurations) and unsigned +on others and therefore unsuitable for writing portable code when every +bit counts, even if they happen to work on your system. diff --git a/docs/code-quality/index.rst b/docs/code-quality/index.rst new file mode 100644 index 0000000000..97939211df --- /dev/null +++ b/docs/code-quality/index.rst @@ -0,0 +1,184 @@ +Code quality +============ + +Because Firefox is a complex piece of software, a lot of tools are +executed to identify issues at development phase. +In this document, we try to list these all tools. + + +.. toctree:: + :maxdepth: 1 + :glob: + + static-analysis.rst + lint/index.rst + coding-style/index.rst + +.. list-table:: C/C++ + :header-rows: 1 + :widths: 20 20 20 20 20 + + * - Tools + - Has autofixes + - Meta bug + - More info + - Upstream + * - Custom clang checker + - + - + - `Source `_ + - + * - Clang-Tidy + - Yes + - `bug 712350 `__ + - :ref:`Static analysis ` + - https://clang.llvm.org/extra/clang-tidy/checks/list.html + * - Clang analyzer + - + - `bug 712350 `__ + - + - https://clang-analyzer.llvm.org/ + * - Coverity + - + - `bug 1230156 `__ + - + - + * - cpp virtual final + - + - + - :ref:`cpp virtual final` + - + * - Semmle/LGTM + - + - `bug 1458117 `__ + - + - + * - clang-format + - Yes + - `bug 1188202 `__ + - :ref:`Formatting C++ Code With clang-format` + - https://clang.llvm.org/docs/ClangFormat.html + +.. list-table:: JavaScript + :widths: 20 20 20 20 20 + :header-rows: 1 + + * - Tools + - Has autofixes + - Meta bug + - More info + - Upstream + * - Eslint + - Yes + - `bug 1229856 `__ + - :ref:`ESLint` + - https://eslint.org/ + * - Mozilla ESLint + - + - `bug 1229856 `__ + - :ref:`Mozilla ESLint Plugin` + - + * - Prettier + - Yes + - `bug 1558517 `__ + - :ref:`JavaScript Coding style` + - https://prettier.io/ + + + +.. list-table:: Python + :widths: 20 20 20 20 20 + :header-rows: 1 + + * - Tools + - Has autofixes + - Meta bug + - More info + - Upstream + * - Flake8 + - Yes (with `autopep8 `_) + - `bug 1155970 `__ + - :ref:`Flake8` + - http://flake8.pycqa.org/ + * - black + - Yes + - `bug 1555560 `__ + - :ref:`black` + - https://black.readthedocs.io/en/stable + * - pylint + - + - `bug 1623024 `__ + - :ref:`pylint` + - https://www.pylint.org/ + * - Python 2/3 compatibility check + - + - `bug 1496527 `__ + - :ref:`Python 2/3 compatibility check` + - + + +.. list-table:: Rust + :widths: 20 20 20 20 20 + :header-rows: 1 + + * - Tools + - Has autofixes + - Meta bug + - More info + - Upstream + * - Rustfmt + - Yes + - `bug 1454764 `__ + - :ref:`Rustfmt` + - https://github.com/rust-lang/rustfmt + * - Clippy + - + - `bug 1361342 `__ + - :ref:`clippy` + - https://github.com/rust-lang/rust-clippy + +.. list-table:: Java + :widths: 20 20 20 20 20 + :header-rows: 1 + + * - Tools + - Has autofixes + - Meta bug + - More info + - Upstream + * - Infer + - + - `bug 1175203 `__ + - + - https://github.com/facebook/infer + +.. list-table:: Others + :widths: 20 20 20 20 20 + :header-rows: 1 + + * - Tools + - Has autofixes + - Meta bug + - More info + - Upstream + * - shellcheck + - + - + - + - https://www.shellcheck.net/ + * - rstchecker + - + - + - :ref:`RST Linter` + - https://github.com/myint/rstcheck + * - Typo detection + - Yes + - + - :ref:`Codespell` + - https://github.com/codespell-project/codespell + * - YAML linter + - + - + - + - https://github.com/adrienverge/yamllint + diff --git a/docs/code-quality/lint/create.rst b/docs/code-quality/lint/create.rst new file mode 100644 index 0000000000..066cb5eeef --- /dev/null +++ b/docs/code-quality/lint/create.rst @@ -0,0 +1,329 @@ +Adding a New Linter to the Tree +=============================== + +Linter Requirements +------------------- + +For a linter to be integrated into the mozilla-central tree, it needs to have: + +* Any required dependencies should be installed as part of ``./mach bootstrap`` +* A ``./mach lint`` interface +* Running ``./mach lint`` command must pass (note, linters can be disabled for individual directories) +* Taskcluster/Treeherder integration +* In tree documentation (under ``docs/code-quality/lint``) to give a basic summary, links and any other useful information +* Unit tests (under ``tools/lint/test``) to make sure that the linter works as expected and we don't regress. + +The review group in Phabricator is ``#linter-reviewers``. + +Linter Basics +------------- + +A linter is a yaml file with a ``.yml`` extension. Depending on how the type of linter, there may +be python code alongside the definition, pointed to by the 'payload' attribute. + +Here's a trivial example: + +no-eval.yml + +.. code-block:: yaml + + EvalLinter: + description: Ensures the string eval doesn't show up. + extensions: ['js'] + type: string + payload: eval + +Now ``no-eval.yml`` gets passed into :func:`LintRoller.read`. + + +Linter Types +------------ + +There are four types of linters, though more may be added in the future. + +1. string - fails if substring is found +2. regex - fails if regex matches +3. external - fails if a python function returns a non-empty result list +4. structured_log - fails if a mozlog logger emits any lint_error or lint_warning log messages + +As seen from the example above, string and regex linters are very easy to create, but they +should be avoided if possible. It is much better to use a context aware linter for the language you +are trying to lint. For example, use eslint to lint JavaScript files, use flake8 to lint python +files, etc. + +Which brings us to the third and most interesting type of linter, +external. External linters call an arbitrary python function which is +responsible for not only running the linter, but ensuring the results +are structured properly. For example, an external type could shell out +to a 3rd party linter, collect the output and format it into a list of +:class:`Issue` objects. The signature for this python +function is ``lint(files, config, **kwargs)``, where ``files`` is a list of +files to lint and ``config`` is the linter definition defined in the ``.yml`` +file. + +Structured log linters are much like external linters, but suitable +for cases where the linter code is using mozlog and emits +``lint_error`` or ``lint_warning`` logging messages when the lint +fails. This is recommended for writing novel gecko-specific lints. In +this case the signature for lint functions is ``lint(files, config, logger, +**kwargs)``. + + +Linter Definition +----------------- + +Each ``.yml`` file must have at least one linter defined in it. Here are the supported keys: + +* description - A brief description of the linter's purpose (required) +* type - One of 'string', 'regex' or 'external' (required) +* payload - The actual linting logic, depends on the type (required) +* include - A list of file paths that will be considered (optional) +* exclude - A list of file paths or glob patterns that must not be matched (optional) +* extensions - A list of file extensions to be considered (optional) +* setup - A function that sets up external dependencies (optional) +* support-files - A list of glob patterns matching configuration files (optional) +* find-dotfiles - If set to ``true``, run on dot files (.*) (optional) +* ignore-case - If set to ``true`` and ``type`` is regex, ignore the case (optional) + +In addition to the above, some ``.yml`` files correspond to a single lint rule. For these, the +following additional keys may be specified: + +* message - A string to print on infraction (optional) +* hint - A string with a clue on how to fix the infraction (optional) +* rule - An id string for the lint rule (optional) +* level - The severity of the infraction, either 'error' or 'warning' (optional) + +For structured_log lints the following additional keys apply: + +* logger - A StructuredLog object to use for logging. If not supplied + one will be created (optional) + + +Example +------- + +Here is an example of an external linter that shells out to the python flake8 linter, +let's call the file ``flake8_lint.py`` (`in-tree version `_): + +.. code-block:: python + + import json + import os + import subprocess + from collections import defaultdict + + from mozlint import result + + try: + from shutil import which + except ImportError: + from shutil_which import which + + + FLAKE8_NOT_FOUND = """ + Could not find flake8! Install flake8 and try again. + """.strip() + + + def lint(files, config, **lintargs): + binary = os.environ.get('FLAKE8') + if not binary: + binary = which('flake8') + if not binary: + print(FLAKE8_NOT_FOUND) + return 1 + + # Flake8 allows passing in a custom format string. We use + # this to help mold the default flake8 format into what + # mozlint's Issue object expects. + cmdargs = [ + binary, + '--format', + '{"path":"%(path)s","lineno":%(row)s,"column":%(col)s,"rule":"%(code)s","message":"%(text)s"}', + ] + files + + proc = subprocess.Popen(cmdargs, stdout=subprocess.PIPE, env=os.environ) + output = proc.communicate()[0] + + # all passed + if not output: + return [] + + results = [] + for line in output.splitlines(): + # res is a dict of the form specified by --format above + res = json.loads(line) + + # parse level out of the id string + if 'code' in res and res['code'].startswith('W'): + res['level'] = 'warning' + + # result.from_linter is a convenience method that + # creates a Issue using a LINTER definition + # to populate some defaults. + results.append(result.from_config(config, **res)) + + return results + +Now here is the linter definition that would call it: + +.. code-block:: yaml + + flake8: + description: Python linter + include: ['.'] + extensions: ['py'] + type: external + payload: py.flake8:lint + support-files: + - '**/.flake8' + +Notice the payload has two parts, delimited by ':'. The first is the module +path, which ``mozlint`` will attempt to import. The second is the object path +within that module (e.g, the name of a function to call). It is up to consumers +of ``mozlint`` to ensure the module is in ``sys.path``. Structured log linters +use the same import mechanism. + +The ``support-files`` key is used to list configuration files or files related +to the running of the linter itself. If using ``--outgoing`` or ``--workdir`` +and one of these files was modified, the entire tree will be linted instead of +just the modified files. + +Result definition +----------------- + +When generating the list of results, the following values are available. + +.. csv-table:: + :header: "Name", "Description", "Optional" + :widths: 20, 40, 10 + + "linter", "Name of the linter that flagged this error", "" + "path", "Path to the file containing the error", "" + "message", "Text describing the error", "" + "lineno", "Line number that contains the error", "" + "column", "Column containing the error", "" + "level", "Severity of the error, either 'warning' or 'error' (default 'error')", "Yes" + "hint", "Suggestion for fixing the error", "Yes" + "source", "Source code context of the error", "Yes" + "rule", "Name of the rule that was violated", "Yes" + "lineoffset", "Denotes an error spans multiple lines, of the form (, )", "Yes" + "diff", "A diff describing the changes that need to be made to the code", "Yes" + + +Automated testing +----------------- + +Every new checker must have tests associated. + +They should be pretty easy to write as most of the work is managed by the Mozlint +framework. The key declaration is the ``LINTER`` variable which must match +the linker declaration. + +As an example, the `Flake8 test `_ looks like the following snippet: + +.. code-block:: python + + import mozunit + LINTER = 'flake8' + + def test_lint_single_file(lint, paths): + results = lint(paths('bad.py')) + assert len(results) == 2 + assert results[0].rule == 'F401' + assert results[1].rule == 'E501' + assert results[1].lineno == 5 + + if __name__ == '__main__': + mozunit.main() + +As always with tests, please make sure that enough positive and negative cases are covered. + +To run the tests: + +.. code-block:: shell + + $ ./mach python-test --python 3 --subsuite mozlint + + +More tests can be `found in-tree `_. + + + +Bootstrapping Dependencies +-------------------------- + +Many linters, especially 3rd party ones, will require a set of dependencies. It +could be as simple as installing a binary from a package manager, or as +complicated as pulling a whole graph of tools, plugins and their dependencies. + +Either way, to reduce the burden on users, linters should strive to provide +automated bootstrapping of all their dependencies. To help with this, +``mozlint`` allows linters to define a ``setup`` config, which has the same +path object format as an external payload. For example (`in-tree version `_): + +.. code-block:: yaml + + flake8: + description: Python linter + include: ['.'] + extensions: ['py'] + type: external + payload: py.flake8:lint + setup: py.flake8:setup + +The setup function takes a single argument, the root of the repository being +linted. In the case of ``flake8``, it might look like: + +.. code-block:: python + + import subprocess + from distutils.spawn import find_executable + + def setup(root, **lintargs): + # This is a simple example. Please look at the actual source for better examples. + if not find_executable('flake8'): + subprocess.call(['pip', 'install', 'flake8']) + +The setup function will be called implicitly before running the linter. This +means it should return fast and not produce any output if there is no setup to +be performed. + +The setup functions can also be called explicitly by running ``mach lint +--setup``. This will only perform setup and not perform any linting. It is +mainly useful for other tools like ``mach bootstrap`` to call into. + + +Adding the linter to the CI +--------------------------- + +First, the job will have to be declared in Taskcluster. + +This should be done in the `mozlint Taskcluster configuration `_. +You will need to define a symbol, how it is executed and on what kind of change. + +For example, for flake8, the configuration is the following: + +.. code-block:: yaml + + py-flake8: + description: flake8 run over the gecko codebase + treeherder: + symbol: py(f8) + run: + mach: lint -l flake8 -f treeherder -f json:/builds/worker/mozlint.json + when: + files-changed: + - '**/*.py' + - '**/.flake8' + # moz.configure files are also Python files. + - '**/*.configure' + +If the linter requires an external program, you will have to install it in the `setup script `_ +and maybe install the necessary files in the `Docker configuration `_. + +.. note:: + + If the defect found by the linter is minor, make sure that it is run as `tier 2 `_. + This prevents the tree from closing because of a tiny issue. + For example, the typo detection is run as tier-2. diff --git a/docs/code-quality/lint/index.rst b/docs/code-quality/lint/index.rst new file mode 100644 index 0000000000..1c282f60b4 --- /dev/null +++ b/docs/code-quality/lint/index.rst @@ -0,0 +1,38 @@ +Linting +======= + +Linters are used in mozilla-central to help enforce coding style and avoid bad practices. Due to the +wide variety of languages in use, this is not always an easy task. In addition, linters should be runnable from editors, from the command line, from review tools +and from continuous integration. It's easy to see how the complexity of running all of these +different kinds of linters in all of these different places could quickly balloon out of control. + +``Mozlint`` is a library that accomplishes several goals: + +1. It provides a standard method for adding new linters to the tree, which can be as easy as + defining a config object in a ``.yml`` file. This helps keep lint related code localized, and + prevents different teams from coming up with their own unique lint implementations. +2. It provides a streamlined interface for running all linters at once. Instead of running N + different lint commands to test your patch, a single ``mach lint`` command will automatically run + all applicable linters. This means there is a single API surface that other tools can use to + invoke linters. +3. With a simple taskcluster configuration, Mozlint provides an easy way to execute all these jobs + at review phase. + +``Mozlint`` isn't designed to be used directly by end users. Instead, it can be consumed by things +like mach, phabricator and taskcluster. + +Getting Help +------------ + +If you need help or have questions, please don’t hesitate to contact us via Matrix +in the "Lint and Formatting" room +(`#lint:mozilla.org `_). + +.. toctree:: + :caption: Linting User Guide + :maxdepth: 1 + :glob: + + usage + create + linters/* diff --git a/docs/code-quality/lint/linters/black.rst b/docs/code-quality/lint/linters/black.rst new file mode 100644 index 0000000000..da4c1a52a2 --- /dev/null +++ b/docs/code-quality/lint/linters/black.rst @@ -0,0 +1,36 @@ +Black +===== + +`Black `__ is a opinionated python code formatter. + + +Run Locally +----------- + +The mozlint integration of black can be run using mach: + +.. parsed-literal:: + + $ mach lint --linter black + +Alternatively, omit the ``--linter black`` and run all configured linters, which will include +black. + + +Configuration +------------- + +To enable black on new directory, add the path to the include +section in the `black.yml `_ file. + +Autofix +------- + +The black linter provides a ``--fix`` option. + + +Sources +------- + +* `Configuration (YAML) `_ +* `Source `_ diff --git a/docs/code-quality/lint/linters/clang-format.rst b/docs/code-quality/lint/linters/clang-format.rst new file mode 100644 index 0000000000..2913c7440e --- /dev/null +++ b/docs/code-quality/lint/linters/clang-format.rst @@ -0,0 +1,35 @@ +clang-format +============ + +`clang-format `__ is a tool to reformat C/C++ to the right coding style. + +Run Locally +----------- + +The mozlint integration of clang-format can be run using mach: + +.. parsed-literal:: + + $ mach lint --linter clang-format + + +Configuration +------------- + +To enable clang-format on new directory, add the path to the include +section in the `clang-format.yml `_ file. + +While excludes: will work, this linter will read the ignore list from `.clang-format-ignore file `_ +at the root directory. This because it is also used by the ./mach clang-format -p command. + +Autofix +------- + +clang-format can reformat the code with the option `--fix` (based on the upstream option `-i`). +To highlight the results, we are using the ``--dry-run`` option (from clang-format 10). + +Sources +------- + +* `Configuration (YAML) `_ +* `Source `_ diff --git a/docs/code-quality/lint/linters/clippy.rst b/docs/code-quality/lint/linters/clippy.rst new file mode 100644 index 0000000000..728b38b6b4 --- /dev/null +++ b/docs/code-quality/lint/linters/clippy.rst @@ -0,0 +1,42 @@ +clippy +====== + +`clippy`_ is the tool for Rust static analysis. + +Run Locally +----------- + +The mozlint integration of clippy can be run using mach: + +.. parsed-literal:: + + $ mach lint --linter clippy + +.. note:: + + clippy expects a path or a .rs file. It doesn't accept Cargo.toml + as it would break the mozlint workflow. + +Configuration +------------- + +To enable clippy on new directory, add the path to the include +section in the `clippy.yml `_ file. + +Autofix +------- + +This linter provides a ``--fix`` option. It requires using nightly +which can be installed with: + +.. code-block:: shell + + $ rustup component add clippy --toolchain nightly-x86_64-unknown-linux-gnu + + + +Sources +------- + +* `Configuration (YAML) `_ +* `Source `_ diff --git a/docs/code-quality/lint/linters/codespell.rst b/docs/code-quality/lint/linters/codespell.rst new file mode 100644 index 0000000000..e5804d30bb --- /dev/null +++ b/docs/code-quality/lint/linters/codespell.rst @@ -0,0 +1,36 @@ +Codespell +========= + +`codespell `__ is a popular tool to look for typical typos in the source code. + +It is enabled mostly for the documentation and English locale files. + +Run Locally +----------- + +The mozlint integration of codespell can be run using mach: + +.. parsed-literal:: + + $ mach lint --linter codespell + + +Configuration +------------- + +To enable codespell on new directory, add the path to the include +section in the `codespell.yml `_ file. + +This job is configured as `tier 2 `_. + +Autofix +------- + +Codespell provides a ``--fix`` option. It is based on the ``-w`` option provided by upstream. + + +Sources +------- + +* `Configuration (YAML) `_ +* `Source `_ diff --git a/docs/code-quality/lint/linters/cpp-virtual-final.rst b/docs/code-quality/lint/linters/cpp-virtual-final.rst new file mode 100644 index 0000000000..5353b6b4fe --- /dev/null +++ b/docs/code-quality/lint/linters/cpp-virtual-final.rst @@ -0,0 +1,30 @@ +cpp virtual final +================= + +This linter detects the virtual function declarations with multiple specifiers. + +It matches our coding style: +Method declarations must use at most one of the following keywords: virtual, override, or final. + +As this linter uses some simple regular expression, it can miss some declarations. + +Run Locally +----------- + +This linter can be run using mach: + +.. parsed-literal:: + + $ mach lint --linter cpp-virtual-final + + +Configuration +------------- + +This linter is enabled on all C family files. + +Sources +------- + +* `Configuration (YAML) `_ + diff --git a/docs/code-quality/lint/linters/eslint-plugin-mozilla.rst b/docs/code-quality/lint/linters/eslint-plugin-mozilla.rst new file mode 100644 index 0000000000..bd844f752a --- /dev/null +++ b/docs/code-quality/lint/linters/eslint-plugin-mozilla.rst @@ -0,0 +1,243 @@ +===================== +Mozilla ESLint Plugin +===================== + +This is the documentation of Mozilla ESLint PLugin. + +Environments +============ + +The plugin implements the following environments: + + +.. toctree:: + :maxdepth: 2 + + eslint-plugin-mozilla/environment + +Rules +===== + +The plugin implements the following rules: + +.. toctree:: + :maxdepth: 1 + + eslint-plugin-mozilla/avoid-Date-timing + eslint-plugin-mozilla/no-define-cc-etc + eslint-plugin-mozilla/no-throw-cr-literal + eslint-plugin-mozilla/no-useless-parameters + eslint-plugin-mozilla/use-chromeutils-import + +avoid-removeChild +----------------- + +Rejects using element.parentNode.removeChild(element) when element.remove() +can be used instead. + +balanced-listeners +------------------ + +Checks that for every occurrence of 'addEventListener' or 'on' there is an +occurrence of 'removeEventListener' or 'off' with the same event name. + +consistent-if-bracing +--------------------- + +Checks that if/elseif/else bodies are braced consistently, so either all bodies +are braced or unbraced. Doesn't enforce either of those styles though. + +import-browser-window-globals +----------------------------- + +For scripts included in browser-window, this will automatically inject the +browser-window global scopes into the file. + +import-content-task-globals +--------------------------- + +For files containing ContentTask.spawn calls, this will automatically declare +the frame script variables in the global scope. ContentTask is only available +to test files, so by default the configs only specify it for the mochitest based +configurations. + +This saves setting the file as a mozilla/frame-script environment. + +Note: due to the way ESLint works, it appears it is only easy to declare these +variables on a file global scope, rather than function global. This may mean that +they are incorrectly allowed, but given they are test files, this should be +detected during testing. + +import-globals +-------------- + +Checks the filename of imported files e.g. ``Cu.import("some/path/Blah.jsm")`` +adds Blah to the global scope. + +Note: uses modules.json for a list of globals listed in each file. + + +import-globals-from +------------------- + +Parses a file for globals defined in various unique Mozilla ways. + +When a "import-globals-from " comment is found in a file, then all globals +from the file at will be imported in the current scope. This will also +operate recursively. + +This is useful for scripts that are loaded as