fix: refactor to avoid UAF in NodeStreamLoader

[dsanders11]

Description of Change

Under specific circumstances NodeStreamLoader can hit a UAF when it deletes itself before completing NodeStreamLoader::NotifyReadable, so refactor to avoid that possibility.

Test added as a separate commit first to show the ASan failure flagging the UAF: https://github.com/electron/electron/actions/runs/30314440929/job/90138979629

Also added tests for two other edge cases which get fixed by the refactor, which would cause the streaming response to stall out: https://github.com/electron/electron/actions/runs/30321732441/job/90160483107

Checklist

• I have built and tested this change
• I have filled out the PR description
• I have reviewed and verified the changes
• npm test passes
• tests are changed or added
• PR release notes describe the change in a way relevant to app developers, and are capitalized, punctuated, and past tense.

Release Notes

Notes: Fixed a UAF with protocol.registerStreamProtocol when an error is emitted during a read

[deepak1556]

How was this case identified ?

[dsanders11]

@deepak1556, it was flagged by the Clang Static Analyzer, via clang-tidy. I'm looking to enable these checks in our .clang-tidy file and working through the reports locally.

clang-tidy output

../../electron/shell/browser/net/node_stream_loader.cc:86:13: warning: Use of memory after it is released [clang-analyzer-cplusplus.NewDelete]
   86 |   readable_ = true;
      |   ~~~~~~~~~ ^
../../electron/shell/browser/net/node_stream_loader.cc:82:7: note: Assuming field 'readable_' is false
   82 |   if (!readable_)
      |       ^~~~~~~~~~
../../electron/shell/browser/net/node_stream_loader.cc:82:3: note: Taking true branch
   82 |   if (!readable_)
      |   ^
../../electron/shell/browser/net/node_stream_loader.cc:83:5: note: Calling 'NodeStreamLoader::ReadMore'
   83 |     ReadMore();
      |     ^~~~~~~~~~
../../electron/shell/browser/net/node_stream_loader.cc:105:7: note: Assuming field 'is_reading_' is false
  105 |   if (is_reading_) {
      |       ^~~~~~~~~~~
../../electron/shell/browser/net/node_stream_loader.cc:105:3: note: Taking false branch
  105 |   if (is_reading_) {
      |   ^
../../electron/shell/browser/net/node_stream_loader.cc:117:3: note: Control jumps to 'case 0:'  at line 117
  117 |   DCHECK(weak) << "We shouldn't have been destroyed when calling read()";
      |   ^
../../base/check.h:293:3: note: expanded from macro 'DCHECK'
  293 |   LOGGING_CHECK_FUNCTION_IMPL(::logging::CheckError::DCheck(#condition), \
      |   ^
../../base/check.h:225:3: note: expanded from macro 'LOGGING_CHECK_FUNCTION_IMPL'
  225 |   switch (0)                                                 \
      |   ^
../../electron/shell/browser/net/node_stream_loader.cc:117:3: note: '?' condition is true
  117 |   DCHECK(weak) << "We shouldn't have been destroyed when calling read()";
      |   ^
../../base/check.h:293:3: note: expanded from macro 'DCHECK'
  293 |   LOGGING_CHECK_FUNCTION_IMPL(::logging::CheckError::DCheck(#condition), \
      |   ^
../../base/check.h:228:30: note: expanded from macro 'LOGGING_CHECK_FUNCTION_IMPL'
  228 |     if (ANALYZER_ASSUME_TRUE((condition) ? true : false))    \
      |                              ^
../../electron/shell/browser/net/node_stream_loader.cc:117:3: note: Taking true branch
  117 |   DCHECK(weak) << "We shouldn't have been destroyed when calling read()";
      |   ^
../../base/check.h:293:3: note: expanded from macro 'DCHECK'
  293 |   LOGGING_CHECK_FUNCTION_IMPL(::logging::CheckError::DCheck(#condition), \
      |   ^
../../base/check.h:228:5: note: expanded from macro 'LOGGING_CHECK_FUNCTION_IMPL'
  228 |     if (ANALYZER_ASSUME_TRUE((condition) ? true : false))    \
      |     ^
../../electron/shell/browser/net/node_stream_loader.cc:121:29: note: Left side of '||' is true
  121 |   if (!ret.ToLocal(&buffer) || !node::Buffer::HasInstance(buffer)) {
      |                             ^
../../electron/shell/browser/net/node_stream_loader.cc:125:9: note: Assuming field 'has_read_waiting_' is false
  125 |     if (has_read_waiting_) {
      |         ^~~~~~~~~~~~~~~~~
../../electron/shell/browser/net/node_stream_loader.cc:125:5: note: Taking false branch
  125 |     if (has_read_waiting_) {
      |     ^
../../electron/shell/browser/net/node_stream_loader.cc:132:9: note: Assuming field 'pending_result_' is true
  132 |     if (pending_result_) {
      |         ^~~~~~~~~~~~~~~
../../electron/shell/browser/net/node_stream_loader.cc:132:5: note: Taking true branch
  132 |     if (pending_result_) {
      |     ^
../../electron/shell/browser/net/node_stream_loader.cc:133:7: note: Calling 'NodeStreamLoader::NotifyComplete'
  133 |       NotifyComplete(result_);
      |       ^~~~~~~~~~~~~~~~~~~~~~~
../../electron/shell/browser/net/node_stream_loader.cc:91:7: note: Field 'is_reading_' is false
   91 |   if (is_reading_ || is_writing_) {
      |       ^
../../electron/shell/browser/net/node_stream_loader.cc:91:7: note: Left side of '||' is false
../../electron/shell/browser/net/node_stream_loader.cc:91:22: note: Assuming field 'is_writing_' is false
   91 |   if (is_reading_ || is_writing_) {
      |                      ^~~~~~~~~~~
../../electron/shell/browser/net/node_stream_loader.cc:91:3: note: Taking false branch
   91 |   if (is_reading_ || is_writing_) {
      |   ^
../../electron/shell/browser/net/node_stream_loader.cc:101:3: note: Memory is released
  101 |   delete this;
      |   ^~~~~~~~~~~
../../electron/shell/browser/net/node_stream_loader.cc:133:7: note: Returning; memory was released
  133 |       NotifyComplete(result_);
      |       ^~~~~~~~~~~~~~~~~~~~~~~
../../electron/shell/browser/net/node_stream_loader.cc:83:5: note: Returning; memory was released
   83 |     ReadMore();
      |     ^~~~~~~~~~
../../electron/shell/browser/net/node_stream_loader.cc:86:13: note: Use of memory after it is released
   86 |   readable_ = true;
      |   ~~~~~~~~~ ^

[deepak1556]

Awesome, thank you!

[release-clerk]

Release Notes Persisted

Fixed a UAF with protocol.registerStreamProtocol when an error is emitted during a read

[trop]

I have automatically backported this PR to "41-x-y", please check out #52513

[trop]

I have automatically backported this PR to "43-x-y", please check out #52514

[trop]

I have automatically backported this PR to "44-x-y", please check out #52515

[trop]

I have automatically backported this PR to "42-x-y", please check out #52516