Update SSLClientSessionCache to handle stateful memory pressure [chromium/src : main]

0 views
Skip to first unread message

Patrick Monette (Gerrit)

unread,
Dec 3, 2025, 4:13:47 PM (2 days ago) Dec 3
to Matt Mueller, Chromium LUCI CQ, chromium...@chromium.org, gavinp...@chromium.org, net-r...@chromium.org
Attention needed from Matt Mueller

Patrick Monette voted and added 1 comment

Votes added by Patrick Monette

Commit-Queue+1

1 comment

Patchset-level comments
File-level comment, Patchset 3 (Latest):
Patrick Monette . resolved

+Matt for net/ PTAL

Open in Gerrit

Related details

Attention is currently required from:
  • Matt Mueller
Submit Requirements:
  • requirement satisfiedCode-Coverage
  • requirement is not satisfiedCode-Owners
  • requirement is not satisfiedCode-Review
  • requirement is not satisfiedReview-Enforcement
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I487235195a8ac60f978d3fa34fb8198c806e3610
Gerrit-Change-Number: 7224452
Gerrit-PatchSet: 3
Gerrit-Owner: Patrick Monette <pmon...@chromium.org>
Gerrit-Reviewer: Matt Mueller <ma...@chromium.org>
Gerrit-Reviewer: Patrick Monette <pmon...@chromium.org>
Gerrit-Attention: Matt Mueller <ma...@chromium.org>
Gerrit-Comment-Date: Wed, 03 Dec 2025 21:13:42 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
satisfied_requirement
unsatisfied_requirement
open
diffy

Matt Mueller (Gerrit)

unread,
Dec 3, 2025, 9:32:41 PM (2 days ago) Dec 3
to Patrick Monette, Matt Mueller, Chromium LUCI CQ, chromium...@chromium.org, gavinp...@chromium.org, net-r...@chromium.org
Attention needed from Patrick Monette

Matt Mueller added 2 comments

File base/memory/memory_pressure_listener_registry.cc
Line 154, Patchset 4 (Latest): run_loop.Run();
Matt Mueller . unresolved

This method is named as being async no longer async if it's running a loop to wait for the task to have finished

File net/ssl/ssl_client_session_cache.cc
Line 219, Patchset 4 (Latest): cache_.UpdateMaxSize(0);
Matt Mueller . unresolved

Does this actually work? The unittest does not actually test what happens when you try to insert an element after setting the max to zero. lru_cache special cases 0 as NO_AUTO_EVICT, meaning it no longer manages the size at all. (And if it didn't special case 0, it would cause an overflow at the line it does ` ShrinkToSize(max_size_ - 1);`)

Open in Gerrit

Related details

Attention is currently required from:
  • Patrick Monette
Submit Requirements:
    • requirement satisfiedCode-Coverage
    • requirement is not satisfiedCode-Owners
    • requirement is not satisfiedCode-Review
    • requirement is not satisfiedNo-Unresolved-Comments
    • requirement is not satisfiedReview-Enforcement
    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
    Gerrit-MessageType: comment
    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: I487235195a8ac60f978d3fa34fb8198c806e3610
    Gerrit-Change-Number: 7224452
    Gerrit-PatchSet: 4
    Gerrit-Owner: Patrick Monette <pmon...@chromium.org>
    Gerrit-Reviewer: Matt Mueller <ma...@chromium.org>
    Gerrit-Reviewer: Patrick Monette <pmon...@chromium.org>
    Gerrit-Attention: Patrick Monette <pmon...@chromium.org>
    Gerrit-Comment-Date: Thu, 04 Dec 2025 02:32:30 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: No
    satisfied_requirement
    unsatisfied_requirement
    open
    diffy

    Patrick Monette (Gerrit)

    unread,
    Dec 5, 2025, 5:10:15 PM (11 hours ago) Dec 5
    to Chromium Metrics Reviews, Menard, Alexis, Mangesh Ghiware, Sophie Chang, Lei Zhang, Stephen Chenney, Mark Schillaci, Dirk Schulze, (Julie)Jeongeun Kim, Heron Yang, AyeAye, Matt Mueller, Chromium LUCI CQ, chromium...@chromium.org, webauthn...@chromium.org, vakh+safe_br...@chromium.org, drott+bl...@chromium.org, dullweb...@chromium.org, hanxi...@chromium.org, sloboda...@chromium.org, omnibox-...@chromium.org, dtseng...@chromium.org, ayman...@chromium.org, cblume...@chromium.org, cc-...@chromium.org, twelling...@chromium.org, nwoked...@chromium.org, apavlo...@chromium.org, mattsimm...@chromium.org, vasilii+watchlis...@chromium.org, asvitkine...@chromium.org, blink-revie...@chromium.org, christia...@chromium.org, ios-r...@chromium.org, tmartino+tran...@chromium.org, feature-me...@chromium.org, yusufo...@chromium.org, fmalit...@chromium.org, derinel+wat...@google.com, yuzo+...@chromium.org, telemetr...@chromium.org, jdonnel...@chromium.org, devtools...@chromium.org, andysjl...@chromium.org, gcasto+w...@chromium.org, dewitt...@chromium.org, thegreenf...@chromium.org, druber...@chromium.org, chromiumme...@microsoft.com, blink-reviews-p...@chromium.org, peilinwa...@google.com, blink-rev...@chromium.org, ender...@chromium.org, nektar...@chromium.org, kyungjunle...@google.com, language...@chromium.org, trewin...@google.com, penghuan...@chromium.org, extension...@chromium.org, lwinston+watc...@google.com, srahim...@chromium.org, josiah...@chromium.org, ios-revie...@chromium.org, mac-r...@chromium.org, dtapuska+...@chromium.org, blink-...@chromium.org, davidj...@chromium.org, yuezhang...@chromium.org, jmedle...@chromium.org, wychen...@chromium.org, blink-re...@chromium.org, agriev...@chromium.org, mfoltz+wa...@chromium.org, kinuko...@chromium.org, asvitki...@chromium.org, marq+...@chromium.org, msrame...@chromium.org, zackha...@chromium.org, donnd...@chromium.org, filesapp...@chromium.org, gangwu...@chromium.org, jdeblas...@chromium.org, mdjone...@chromium.org, meilian...@chromium.org, chromium-a...@chromium.org, francisjp...@google.com, android-web...@chromium.org, jatapiaro+wat...@google.com, browser-comp...@chromium.org, xinghui...@chromium.org, gogeral...@chromium.org, abigailbk...@google.com, blink-re...@chromium.org, blink-re...@chromium.org, fserb...@chromium.org, drott...@chromium.org, gavinp...@chromium.org, net-r...@chromium.org
    Attention needed from Matt Mueller

    Patrick Monette added 3 comments

    Patchset-level comments
    File-level comment, Patchset 8 (Latest):
    Patrick Monette . resolved

    Thanks for the review. You uncovered a major issue.

    PTAnL

    File base/memory/memory_pressure_listener_registry.cc
    Line 154, Patchset 4: run_loop.Run();
    Matt Mueller . resolved

    This method is named as being async no longer async if it's running a loop to wait for the task to have finished

    Patrick Monette

    Good point. The name is mostly to mean that it's to be used for async memory pressure listeners, but I agree that it doesn't feel right. I ended up with another version in https://chromium-review.googlesource.com/c/chromium/src/+/7229564 where the function is truly async, but you can pass the QuitClosure as a parameter so the synchronous part is done by the calling code.

    File net/ssl/ssl_client_session_cache.cc
    Line 219, Patchset 4: cache_.UpdateMaxSize(0);
    Matt Mueller . resolved

    Does this actually work? The unittest does not actually test what happens when you try to insert an element after setting the max to zero. lru_cache special cases 0 as NO_AUTO_EVICT, meaning it no longer manages the size at all. (And if it didn't special case 0, it would cause an overflow at the line it does ` ShrinkToSize(max_size_ - 1);`)

    Patrick Monette

    Yeah I missed a big issue here. Thanks!

    I needed more changes to LRUCache which I did in this CL: https://chromium-review.googlesource.com/c/chromium/src/+/7228584.

    I also added a test to ensure inserting an element fails when the max is zero.

    Open in Gerrit

    Related details

    Attention is currently required from:
    • Matt Mueller
    Submit Requirements:
      • requirement satisfiedCode-Coverage
      • requirement is not satisfiedCode-Owners
      • requirement is not satisfiedCode-Review
      • requirement is not satisfiedReview-Enforcement
      Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
      Gerrit-MessageType: comment
      Gerrit-Project: chromium/src
      Gerrit-Branch: main
      Gerrit-Change-Id: I487235195a8ac60f978d3fa34fb8198c806e3610
      Gerrit-Change-Number: 7224452
      Gerrit-PatchSet: 8
      Gerrit-Owner: Patrick Monette <pmon...@chromium.org>
      Gerrit-Reviewer: Matt Mueller <ma...@chromium.org>
      Gerrit-Reviewer: Patrick Monette <pmon...@chromium.org>
      Gerrit-CC: (Julie)Jeongeun Kim <je_jul...@chromium.org>
      Gerrit-CC: Akihiro Ota <akihi...@chromium.org>
      Gerrit-CC: Chromium Metrics Reviews <chromium-met...@google.com>
      Gerrit-CC: Dirk Schulze <dsch...@chromium.org>
      Gerrit-CC: Heron Yang <hero...@google.com>
      Gerrit-CC: Lei Zhang <the...@chromium.org>
      Gerrit-CC: Mangesh Ghiware <mghi...@google.com>
      Gerrit-CC: Mark Schillaci <mschi...@google.com>
      Gerrit-CC: Menard, Alexis <alexis...@intel.com>
      Gerrit-CC: Sophie Chang <sophi...@chromium.org>
      Gerrit-CC: Stephen Chenney <sche...@chromium.org>
      Gerrit-Attention: Matt Mueller <ma...@chromium.org>
      Gerrit-Comment-Date: Fri, 05 Dec 2025 22:10:07 +0000
      Gerrit-HasComments: Yes
      Gerrit-Has-Labels: No
      Comment-In-Reply-To: Matt Mueller <ma...@chromium.org>
      satisfied_requirement
      unsatisfied_requirement
      open
      diffy

      Patrick Monette (Gerrit)

      unread,
      Dec 5, 2025, 10:31:27 PM (5 hours ago) Dec 5
      to Chromium Metrics Reviews, Menard, Alexis, Mangesh Ghiware, Sophie Chang, Lei Zhang, Stephen Chenney, Mark Schillaci, Dirk Schulze, (Julie)Jeongeun Kim, Heron Yang, AyeAye, Matt Mueller, Chromium LUCI CQ, chromium...@chromium.org, webauthn...@chromium.org, vakh+safe_br...@chromium.org, drott+bl...@chromium.org, dullweb...@chromium.org, hanxi...@chromium.org, sloboda...@chromium.org, omnibox-...@chromium.org, dtseng...@chromium.org, ayman...@chromium.org, cblume...@chromium.org, cc-...@chromium.org, twelling...@chromium.org, nwoked...@chromium.org, apavlo...@chromium.org, mattsimm...@chromium.org, vasilii+watchlis...@chromium.org, asvitkine...@chromium.org, blink-revie...@chromium.org, christia...@chromium.org, ios-r...@chromium.org, tmartino+tran...@chromium.org, feature-me...@chromium.org, yusufo...@chromium.org, fmalit...@chromium.org, derinel+wat...@google.com, yuzo+...@chromium.org, telemetr...@chromium.org, jdonnel...@chromium.org, devtools...@chromium.org, andysjl...@chromium.org, gcasto+w...@chromium.org, dewitt...@chromium.org, thegreenf...@chromium.org, druber...@chromium.org, chromiumme...@microsoft.com, blink-reviews-p...@chromium.org, peilinwa...@google.com, blink-rev...@chromium.org, ender...@chromium.org, nektar...@chromium.org, kyungjunle...@google.com, language...@chromium.org, trewin...@google.com, penghuan...@chromium.org, extension...@chromium.org, lwinston+watc...@google.com, srahim...@chromium.org, josiah...@chromium.org, ios-revie...@chromium.org, mac-r...@chromium.org, dtapuska+...@chromium.org, blink-...@chromium.org, davidj...@chromium.org, yuezhang...@chromium.org, jmedle...@chromium.org, wychen...@chromium.org, blink-re...@chromium.org, agriev...@chromium.org, mfoltz+wa...@chromium.org, kinuko...@chromium.org, asvitki...@chromium.org, marq+...@chromium.org, msrame...@chromium.org, zackha...@chromium.org, donnd...@chromium.org, filesapp...@chromium.org, gangwu...@chromium.org, jdeblas...@chromium.org, mdjone...@chromium.org, meilian...@chromium.org, chromium-a...@chromium.org, francisjp...@google.com, android-web...@chromium.org, jatapiaro+wat...@google.com, browser-comp...@chromium.org, xinghui...@chromium.org, gogeral...@chromium.org, abigailbk...@google.com, blink-re...@chromium.org, blink-re...@chromium.org, fserb...@chromium.org, drott...@chromium.org, gavinp...@chromium.org, net-r...@chromium.org
      Attention needed from Matt Mueller

      Patrick Monette added 1 comment

      Patchset-level comments
      File-level comment, Patchset 9 (Latest):
      Patrick Monette . resolved

      Unfortunately a bad rebase added a bunch of bots that keep failing, but the CQ passes.

      Open in Gerrit

      Related details

      Attention is currently required from:
      • Matt Mueller
      Submit Requirements:
      • requirement satisfiedCode-Coverage
      • requirement is not satisfiedCode-Owners
      • requirement is not satisfiedCode-Review
      • requirement is not satisfiedReview-Enforcement
      Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
      Gerrit-MessageType: comment
      Gerrit-Project: chromium/src
      Gerrit-Branch: main
      Gerrit-Change-Id: I487235195a8ac60f978d3fa34fb8198c806e3610
      Gerrit-Change-Number: 7224452
      Gerrit-PatchSet: 9
      Gerrit-Owner: Patrick Monette <pmon...@chromium.org>
      Gerrit-Reviewer: Matt Mueller <ma...@chromium.org>
      Gerrit-Reviewer: Patrick Monette <pmon...@chromium.org>
      Gerrit-CC: (Julie)Jeongeun Kim <je_jul...@chromium.org>
      Gerrit-CC: Akihiro Ota <akihi...@chromium.org>
      Gerrit-CC: Chromium Metrics Reviews <chromium-met...@google.com>
      Gerrit-CC: Dirk Schulze <dsch...@chromium.org>
      Gerrit-CC: Heron Yang <hero...@google.com>
      Gerrit-CC: Lei Zhang <the...@chromium.org>
      Gerrit-CC: Mangesh Ghiware <mghi...@google.com>
      Gerrit-CC: Mark Schillaci <mschi...@google.com>
      Gerrit-CC: Menard, Alexis <alexis...@intel.com>
      Gerrit-CC: Sophie Chang <sophi...@chromium.org>
      Gerrit-CC: Stephen Chenney <sche...@chromium.org>
      Gerrit-Attention: Matt Mueller <ma...@chromium.org>
      Gerrit-Comment-Date: Sat, 06 Dec 2025 03:31:17 +0000
      Gerrit-HasComments: Yes
      Gerrit-Has-Labels: No
      satisfied_requirement
      unsatisfied_requirement
      open
      diffy
      Reply all
      Reply to author
      Forward
      0 new messages