GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — src: remove dependency on wrapper-descriptor-based cpp heap by joyeecheung · Pull Request #53086 · nodejs/node

Conversation

@joyeecheung

@joyeecheung joyeecheung commented May 21, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

This is baking for the V8 12.5 upgrade so the base branch is canary-base. Original PR and reviews are in nodejs/node-v8#284

src: remove dependency on wrapper-descriptor-based CppHeap.

As V8 has moved away from wrapper-descriptor-based CppHeap, this
patch:

  1. Create the CppHeap without using wrapper descirptors.
  2. Deprecates node::SetCppgcReference() in favor of
    v8::Object::Wrap() since the wrapper descriptor is no longer
    relevant. It is still kept as a compatibility layer for addons
    that need to also work on Node.js versions without
    v8::Object::Wrap().

Fixes: nodejs/node-v8#283

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bricss reacted with eyes emoji
All reactions
  • 👀 1 reaction
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs involving general changes in the lib/ or src/ directories. needs-ci PRs that need a full CI run. labels May 21, 2024
@joyeecheung joyeecheung mentioned this pull request May 21, 2024
src: remove dependency on wrapper-descriptor-based cpp heap nodejs/node-v8#284
Closed
@joyeecheung joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label May 21, 2024
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label May 21, 2024
@nodejs-github-bot

nodejs-github-bot commented May 21, 2024

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/59334/

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@targos

targos commented May 22, 2024

Copy link
Copy Markdown
Member

Looks like some things are broken.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@joyeecheung

joyeecheung commented May 22, 2024

Copy link
Copy Markdown
Member Author

Looks like we have a timing conflict with #30846 (comment) - if we migrate to the new CppHeap management, the CppHeap is torn down during Isolate disposal, and at this point the Isolate needs to be registered with the Platform for it to look up the task runner. But due to #30846 (comment) we also had to unregister the Isolate before the disposal to avoid the mentioned race condition.

I wonder if this can be worked around by using something else as the Isolate map keys in the platform to avoid the said race condition & allow us to tear down the isolate while it's still discoverable by the platform...maybe using a self-incrementing ID as key is already enough? cc @addaleax

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@addaleax

addaleax commented May 23, 2024

Copy link
Copy Markdown
Member

@joyeecheung Yeah ... I assume in the end any key that we can look up based on the Isolate* pointer, without needing the Isolate instance itself, will work? But obviously the pointer itself is a pretty natural choice ... would it be possible to split Isolate disposal into two parts, like Isolate creation (i.e. split into allocation + initialization)? That requires a V8 change but might be the best solution apart from that

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@joyeecheung

joyeecheung commented May 27, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

I wonder if there's another way to fix the isolate disposal race condition...by shutting down the platform data/task runner after we remove it from the map? In that case it's not possible for the conflicting new Isolate to be mapped to the platform data being shut down since it's already out of the map and is doing its own thing. Locally that seems fine and doesn't regress the test, although I am unable to reproduce the flake in #30846 after reverting the dispose-unregister order either, I'll open a separate PR for this change and investigate.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

targos and others added 16 commits May 28, 2024 10:26
@targos
deps: update V8 to 12.7.137
3566324
@targos
build: reset embedder string to "-node.0"
512a7b9
@targos
src: update NODE_MODULE_VERSION to 128
47be3ba
Major V8 updates are usually API/ABI incompatible with previous
versions. This commit adapts NODE_MODULE_VERSION for V8 12.7.

Refs: https://github.com/nodejs/CTC/blob/master/meetings/2016-09-28.md
@targos
deps: always define V8_EXPORT_PRIVATE as no-op
223de68
dllexport introduces issues when compiling with MSVC.

PR-URL: nodejs#47251
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
@targos
deps: disable V8 concurrent sparkplug compilation
a827b0e
It introduces process hangs on some platforms because Node.js doesn't
tear down V8 correctly.
Disable it while we work on a solution.

Refs: nodejs#47297
Refs: https://bugs.chromium.org/p/v8/issues/detail?id=13902
PR-URL: nodejs#47450
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@targos
deps: avoid compilation error with ASan
9b83b8e
Refs: https://bugs.chromium.org/p/v8/issues/detail?id=14221
PR-URL: nodejs#49639
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
@targos
deps: remove usage of a C++20 feature from V8
3849d83
We are not ready to compile with C++20 support yet.
This is only a DCHECK that can be removed without affecting the behavior
of release builds.

PR-URL: nodejs#49639
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
@targos
deps: patch V8 to avoid duplicated zlib symbol
4da65e9
PR-URL: nodejs#49639
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
@targos
deps: silence internal V8 deprecation warning
35ebfb1
PR-URL: nodejs#52293
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
@StefanStojanovic @targos
deps: patch V8 to support compilation with MSVC
38a5e0b
After enabling -std:c++20 on Windows, patch is now much smaller.

PR-URL: nodejs#52465
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@targos
deps: V8: revert CL 5331688
9d544d4
On Windows debug builds, it is not allowed to dereference empty
iterators.

Refs: https://chromium-review.googlesource.com/c/v8/v8/+/5331688
PR-URL: nodejs#52465
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@targos
build: include v8-sandbox.h header in distribution
9d8823a
Refs: v8/v8@5495678
@fdoray @targos
src: add source location to v8::TaskRunner
8d34ef7
Refs: https://chromium-review.googlesource.com/c/v8/v8/+/5300826
@LeszekSwirski @targos
build: disable ICF for mksnapshot
f553d5d
Refs: https://chromium-review.googlesource.com/c/v8/v8/+/5447267
Co-authored-by: Michaël Zasso <[email protected]>
@targos
test: update v8-stats test for V8 12.6
54ad3e4
Refs: v8/v8@e30e228
@targos
tools: update V8 gypfiles for 12.6
a1c9178
Refs: v8/v8@f8fa220
@targos
targos force-pushed the canary-base branch from 9dc716f to a1c9178 Compare May 28, 2024 09:07
@targos

targos commented May 28, 2024

Copy link
Copy Markdown
Member

I just had to force-push canary-base to remove 9197598 (it's now on main).

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@targos

targos commented May 29, 2024

Copy link
Copy Markdown
Member

v8#187 may be relevant

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@joyeecheung
joyeecheung force-pushed the cppheap-auto branch from 9ea7f9b to 1a14799 Compare May 29, 2024 18:28
@github-actions github-actions Bot added request-ci-failed Starting CI with the request-ci label failed and requires manual intervention. and removed request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. labels May 29, 2024
@github-actions

github-actions Bot commented May 29, 2024

Copy link
Copy Markdown
Contributor
Failed to start CI
   ⚠  No approving reviews found
   ✘  Refusing to run CI on potentially unsafe PR
https://github.com/nodejs/node/actions/runs/9291289224
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@nodejs-github-bot

nodejs-github-bot commented May 29, 2024

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/59525/

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@joyeecheung

joyeecheung commented May 29, 2024

Copy link
Copy Markdown
Member Author

Pushed a commit here to fix the -Werror=comment errors in v8-sandbox.h to see if Linux CI is happy with it..

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@joyeecheung joyeecheung mentioned this pull request May 29, 2024
src: use V8-owned CppHeap #53205
Closed
jasnell
jasnell reviewed May 29, 2024
View reviewed changes
Comment thread src/env.cc Resolved

Uh oh!

There was an error while loading. Please reload this page.

jasnell
jasnell reviewed May 29, 2024
View reviewed changes
Comment thread src/env.cc Outdated

@jasnell jasnell May 29, 2024

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to clarify for myself on this comment... this means you're still setting an embedder_id in slot 0 in base objects in order to differentiate our own API objects from any other v8::Object for which IsApiWrapper() returns true, correct?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@joyeecheung joyeecheung May 30, 2024

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, basically for printing things and snapshots.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
jasnell
jasnell reviewed May 29, 2024
View reviewed changes
Comment thread src/env.cc Resolved

Uh oh!

There was an error while loading. Please reload this page.

jasnell
jasnell reviewed May 29, 2024
View reviewed changes
Comment thread src/node.h Outdated

@jasnell jasnell May 29, 2024

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For this, should we also re-export the enum value to be used with v8::Object::Wrap/Unwrap to make it easier? It's fairly non-obvious that folks would need to add an include to v8-sandbox.h to get that. Alternatively, we could consider exposing our own wrapper wrap/unwrap methods here that delegate to the new v8 APIs to hide that detail.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@joyeecheung joyeecheung May 30, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's fairly non-obvious that folks would need to add an include to v8-sandbox.h to get that.

That sounds more like something that should be documented in V8's header?

Alternatively, we could consider exposing our own wrapper wrap/unwrap methods here that delegate to the new v8 APIs to hide that detail.

For our side I think that can be left as a TODO, this is mostly for just migrating away from the existing deprecated APIs. SetCppgcReference() is already basically just v8::Object::Wrap, we can add a new API to provide GetCppgcReference() in a different PR but it feels out of the place for this one (which is for canary-base, not main).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@targos

targos commented May 30, 2024

Copy link
Copy Markdown
Member

I don't know if it's related but there are many crashes with heapdump/heapsnapshot tests: https://ci.nodejs.org/job/node-test-commit-arm-debug/13095/

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@joyeecheung

joyeecheung commented May 30, 2024

Copy link
Copy Markdown
Member Author

The heap snapshot failures are the same as nodejs/node-v8#282 which are being fixed in https://chromium-review.googlesource.com/c/v8/v8/+/5531355

targos reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@joyeecheung

joyeecheung commented May 30, 2024

Copy link
Copy Markdown
Member Author

v8-sandbox.h fixes upstreamed in https://chromium-review.googlesource.com/c/v8/v8/+/5584738

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

hubot pushed a commit to v8/v8 that referenced this pull request May 30, 2024
@joyeecheung V8 LUCI CQ
src: fix v8-sandbox.h multiline comments
300451e
Before it fails to compile with -Werrors=comment because some of
the `//` comments uses `\` at the end of the line.

Refs: nodejs/node#53086

Change-Id: I10e5b970cff4a8eda78c305f5649d8bf08d7da3c
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/5584738
Reviewed-by: Michael Lippautz <[email protected]>
Commit-Queue: Joyee Cheung <[email protected]>
Cr-Commit-Position: refs/heads/main@{#94165}
joyeecheung added 2 commits May 30, 2024 15:45
@joyeecheung
deps: V8: cherry-pick 300451e869ae
cb18042
Original commit message:

    src: fix v8-sandbox.h multiline comments

    Before it fails to compile with -Werrors=comment because some of
    the `//` comments uses `\` at the end of the line.

    Refs: nodejs#53086

    Change-Id: I10e5b970cff4a8eda78c305f5649d8bf08d7da3c
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/5584738
    Reviewed-by: Michael Lippautz <[email protected]>
    Commit-Queue: Joyee Cheung <[email protected]>
    Cr-Commit-Position: refs/heads/main@{#94165}

Refs: v8/v8@300451e
@joyeecheung
src: remove dependency on wrapper-descriptor-based CppHeap
6c7c7c5
As V8 has moved away from wrapper-descriptor-based CppHeap, this
patch:

1. Create the CppHeap without using wrapper descirptors.
2. Deprecates node::SetCppgcReference() in favor of
   v8::Object::Wrap() since the wrapper descriptor is no longer
   relevant. It is still kept as a compatibility layer for addons
   that need to also work on Node.js versions without
   v8::Object::Wrap().
@joyeecheung
joyeecheung force-pushed the cppheap-auto branch from d48d61a to 6c7c7c5 Compare May 30, 2024 13:45
@nodejs-github-bot

nodejs-github-bot commented Jun 1, 2024

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/59598/

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@joyeecheung

joyeecheung commented Jun 4, 2024

Copy link
Copy Markdown
Member Author

Not sure what's the way to make progress here...the CI failures are unrelated AFAICT, but the test won't be passing since canary-base isn't working yet. What's the policy for landing patches on canary-base?

Also cc @addaleax @legendecas who reviewed the original PR.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@targos

targos commented Jun 4, 2024

Copy link
Copy Markdown
Member

There isn't really a policy. From my PoV, if it is brings us towards a passing canary build, this can land.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

legendecas
legendecas approved these changes Jun 4, 2024
View reviewed changes
@targos
targos force-pushed the canary-base branch from a1c9178 to 30329d0 Compare June 4, 2024 11:22
@targos

targos commented Jun 4, 2024

Copy link
Copy Markdown
Member

I updated canary-base so it includes v8/v8@300451e
and pushed 30329d0 to it.
Thank you!

joyeecheung reacted with heart emoji
All reactions
  • ❤️ 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@targos targos closed this Jun 4, 2024
@targos targos mentioned this pull request Jun 4, 2024
v8::WrapperDescriptor is deprecated nodejs/node-v8#283
Closed
jkleinsc added a commit to electron/electron that referenced this pull request Jun 4, 2024
@jkleinsc
src: remove dependency on wrapper-descriptor-based cpp heap
747dc81
nodejs/node#53086
jkleinsc added a commit to electron/electron that referenced this pull request Jun 6, 2024
@jkleinsc
src: remove dependency on wrapper-descriptor-based cpp heap
97f37bd
nodejs/node#53086
jkleinsc added a commit to electron/electron that referenced this pull request Jun 7, 2024
@jkleinsc
src: remove dependency on wrapper-descriptor-based cpp heap
7b70c5d
nodejs/node#53086
jkleinsc added a commit to electron/electron that referenced this pull request Jun 7, 2024
@electron-roller @VerteDinde
@nornagon @clavin @ckerr @patchup @jkleinsc @deepak1556
chore: bump chromium to 127.0.6521.0 (main) (#42118)
c3b4cd9
* chore: bump chromium in DEPS to 126.0.6470.0

* 5492605: Migrate TODOs referencing old crbug IDs to the new issue tracker IDs | https://chromium-review.googlesource.com/c/chromium/src/+/5492605

* 5513277: Move subresource-filter-ruleset to GCS | https://chromium-review.googlesource.com/c/chromium/src/+/5513277

* 5512656: Remove CustomizeChromeSupportsChromeRefresh2023 | https://chromium-review.googlesource.com/c/chromium/src/+/5512656

* 5516009: Accept mouse events in inactive window for Top Chrome WebUIs | https://chromium-review.googlesource.com/c/chromium/src/+/5516009

* 5376861: Change references to RWHVB in RWHIER and RenderWidgetTargeter to RWHVI. | https://chromium-review.googlesource.com/c/chromium/src/+/5376861

* 5490530: Use partition_alloc PA_BUILDFLAG(...) outside PA. #cleanup | https://chromium-review.googlesource.com/c/chromium/src/+/5490530

* 5296870: network: Allow trusted loaders to learn the sent request cookies. | https://chromium-review.googlesource.com/c/chromium/src/+/5296870

* 5453438: Delegate delegated ink trails to RWHI from RWHIER. | https://chromium-review.googlesource.com/c/chromium/src/+/5453438

* chore: update patches

* chore: bump chromium in DEPS to 126.0.6472.0

* chore: bump chromium in DEPS to 126.0.6474.0

* chore: update patches

* chore: bump chromium in DEPS to 126.0.6476.0

* chore: bump chromium in DEPS to 126.0.6478.0

* chore: bump chromium in DEPS to 126.0.6478.3

* chore: bump chromium in DEPS to 126.0.6478.8

* update patches

* only disable enterprise_cloud_content_analysis

* 5403888: [api] support v8::Data in v8::TracedReference and v8::EmbedderGraph

https://chromium-review.googlesource.com/c/v8/v8/+/5403888

* chore: bump chromium in DEPS to 127.0.6484.0

* chore: bump chromium in DEPS to 127.0.6485.0

* 5539004: Use NOTREACHED_IN_MIGRATION() in remaining chrome/ | https://chromium-review.googlesource.com/c/chromium/src/+/5539004

* src: cast to v8::Value before using v8::EmbedderGraph::V8Node | https://github.com/nodejs/node/pull/52638/files

* chore: update patches

* chore: update v8 patches

* chore: bump chromium in DEPS to 127.0.6486.0

* chore: bump chromium in DEPS to 127.0.6488.0

* chore: bump chromium in DEPS to 127.0.6490.0

* chore: bump chromium in DEPS to 127.0.6492.0

* chore: update patches

For some reason, `feat_expose_raw_response_headers_from_urlloader.patch` got messed up in an earlier commit.

* chore: update patches

printing.patch was updated due to https://chromium-review.googlesource.com/c/chromium/src/+/5535938

* 5527572: Move Connectors prefs files to components/enterprise/connectors/

https://chromium-review.googlesource.com/c/chromium/src/+/5527572

* chore: bump chromium in DEPS to 127.0.6494.0

* chore: bump chromium in DEPS to 127.0.6495.0

* chore: bump chromium in DEPS to 127.0.6496.0

* 5465511: [api] Mark v8::ObjectTemplate::SetAccessor(..) for deprecation
https://chromium-review.googlesource.com/c/v8/v8/+/5465511

* chore: revert v8 deprecation

See patch message for more details.

https://chromium-review.googlesource.com/c/v8/v8/+/5526611

* chore: update patches

* 5538771: Remove srcdoc else-if block in CalculateOrigin()
https://chromium-review.googlesource.com/c/chromium/src/+/5538771

* 5522321: [devtools] Support saving base64 encoded files via host bindings
https://chromium-review.googlesource.com/c/chromium/src/+/5522321

* 5376861: Change references to RWHVB in RWHIER and RenderWidgetTargeter to RWHVI.
https://chromium-review.googlesource.com/c/chromium/src/+/5376861

* 5530163: [media] Use VideoFrame::Plane typed enum instead of nameless enum
https://chromium-review.googlesource.com/c/chromium/src/+/5530163

* 5463431: iwa: Only create IsolatedWebAppURLLoaderFactory for subresources in IWAs
https://chromium-review.googlesource.com/c/chromium/src/+/5463431

* fixup! 5465511: [api] Mark v8::ObjectTemplate::SetAccessor(..) for deprecation https://chromium-review.googlesource.com/c/v8/v8/+/5465511

* 5512176: Remove OnEnvironmentEstimationComplete()
https://chromium-review.googlesource.com/c/chromium/src/+/5512176

* 5528282: Move Web Speech API .mojom files to //media/mojo/mojom
https://chromium-review.googlesource.com/c/chromium/src/+/5528282

* 5513740: Reland "[Extensions] Restructure extensions::ProcessMap"
https://chromium-review.googlesource.com/c/chromium/src/+/5513740

* 5483406: [PEPC] Make PEPC permission subscription take into account device status
https://chromium-review.googlesource.com/c/chromium/src/+/5483406

* 5526034: [DoH] Remove kDnsOverHttps feature flag
https://chromium-review.googlesource.com/c/chromium/src/+/5526034

The title is a bit misleading. They removed handling for the feature flag and generally intend to remove it but haven't yet.

I only changed our code to address the flag that was removed. A quick search on GitHub for `DnsOverHttpsFallback` yielded a few results, but they were all C++ chromium code or patches, 0 app code or discussion results. Since I couldn't find any evidence of this flag being used in developer applications, I've chosen to exclude this change from the breaking changes docs.

* chore: revert v8 removal

https://chromium-review.googlesource.com/c/v8/v8/+/5497515

See patch message for more details.

* chore: cherry-pick Node.js patch for V8 API removal fix

Node.js PR: nodejs/node#52996
V8 API Removal CL: https://chromium-review.googlesource.com/c/v8/v8/+/5539888

See the patch description for more details.

* 5492183: Extensions: CodeHealth: Give enums some class
https://chromium-review.googlesource.com/c/chromium/src/+/5492183

* fixup! 5528282: Move Web Speech API .mojom files to //media/mojo/mojom https://chromium-review.googlesource.com/c/chromium/src/+/5528282

* 5514687: Reland "Add a secret handshake to the base::Feature constructor"
https://chromium-review.googlesource.com/c/chromium/src/+/5514687

* fixup! 5530163: [media] Use VideoFrame::Plane typed enum instead of nameless enum https://chromium-review.googlesource.com/c/chromium/src/+/5530163

* 5466238: PDF Viewer: add metrics to record if PDF is opened with a11y
https://chromium-review.googlesource.com/c/chromium/src/+/5466238

* 5502081: Migrate OnDisplayRemoved to OnDisplaysRemoved
https://chromium-review.googlesource.com/c/chromium/src/+/5502081

* 5539888: [api] Remove several APIs deprecated in version 12.6
https://chromium-review.googlesource.com/c/v8/v8/+/5539888

This commit essentially only removes the `only_terminate_in_safe_scope` isolate creation parameter. This undoes some work that was originally done in #35766.

* 5498236: Make browser_tests force full async initialization for OSCrypt Async
https://chromium-review.googlesource.com/c/chromium/src/+/5498236

* fixup! 5528282: Move Web Speech API .mojom files to //media/mojo/mojom https://chromium-review.googlesource.com/c/chromium/src/+/5528282

* 5545807: Migrate most remaining NOTREACHED()
https://chromium-review.googlesource.com/c/chromium/src/+/5545807

I took a systematic approach to modifying all of our uses of `NOTREACHED` that were causing errors:
* If there was a `return` or `break` (etc.) immediately after `NOTREACHED`, I removed the control flow instruction and left the `NOTREACHED` unmodified
* All other instances were migrated to `NOTREACHED_IN_MIGRATION`

We should revisit pretty much all usage of `NOTREACHED` as an upgrade follow-up item.

* fixup! 5526034: [DoH] Remove kDnsOverHttps feature flag https://chromium-review.googlesource.com/c/chromium/src/+/5526034

Turns out the feature flags were removed in the `.cc` file, but not the
`.h` feature list file. This means that the feature flags are pretty
much officially gone. (The leftover symbols in the header are likely an
oversight from what I can gather.)

We may potentially decide to put this in the breaking changes doc if we
decide this feature flag is important enough to highlight.

* chore: bump chromium in DEPS to 127.0.6498.3

* chore: bump chromium in DEPS to 127.0.6500.0

* chore: bump chromium in DEPS to 127.0.6502.0

* chore: bump chromium in DEPS to 127.0.6504.0

* chore: bump chromium in DEPS to 127.0.6505.0

* chore: bump chromium in DEPS to 127.0.6508.0

* build: use Sha256Sum in script/sysroots.json

Xref: https://chromium-review.googlesource.com/c/chromium/src/+/5506275

* chore: update chore_add_electron_deps_to_gitignores.patch

Xref: no manual changes; patch applied with fuzz 2

* chore: update feat_allow_code_cache_in_custom_schemes.patch

Xref: no manual changes; patch applied with fuzz 1

* chore: e patches all

* fixup! build: use Sha256Sum in script/sysroots.json

`sync` succeeds now

* chore: replace absl::optional with std::optional

Xref: https://chromium-review.googlesource.com/c/chromium/src/+/5253843

* chore: update CalculatePreferredSize() to new upstream semantics

Xref: https://chromium-review.googlesource.com/c/chromium/src/+/5459174
Xref: https://chromium-review.googlesource.com/c/chromium/src/+/5541220
Xref: https://chromium-review.googlesource.com/c/chromium/src/+/5514708
Xref: https://chromium-review.googlesource.com/c/chromium/src/+/5504212
Xref: https://chromium-review.googlesource.com/516542

* chore: replace absl::optional with std::optional

Xref: https://chromium-review.googlesource.com/c/chromium/src/+/5296147

* chore: add kPip to enumeration as a no-op

https://chromium-review.googlesource.com/c/chromium/src/+/5546257

* [Autofill] Remove RenderFrame::ElementBoundsInWindow()

Xref: https://chromium-review.googlesource.com/c/chromium/src/+/5553982

* chore: fix feat_add_streaming-protocol_registry_to_multibuffer_data_source.patch

need new header to pick up definition of BLINK_PLATFORM_EXPORT macro

Xref: https://chromium-review.googlesource.com/c/chromium/src/+/5463143

* chore: bump chromium in DEPS to 127.0.6510.0

* chore: update patches

* chore: fix include path for native_web_keyboard_event.h

Xref: https://chromium-review.googlesource.com/c/chromium/src/+/5541976

* chore: add currently-unused should_include_device_status arg to GetPermissionStatusForCurrentDocument()

Xref: https://chromium-review.googlesource.com/c/chromium/src/+/5545382

* chore: bump chromium in DEPS to 127.0.6512.0

* chore: update mas_avoid_private_macos_api_usage.patch.patch

No manual changes; patch applied with fuzz 1

* chore: update feat_add_streaming-protocol_registry_to_multibuffer_data_source.patch

No manual changes; patch applied with fuzz 1

* chore: update webview_fullscreen.patch

No manual changes; patch applied with fuzz 1

* chore=: remove cherry-pick-22db6918bac9.patch

already present upstream

* chore: remove nonexistent patchfiles from .patches

* chore: remove cherry-pick-3e037e195e50.patch

no longer needed; merged upstream

* Update namespace for files moved to //components/input

Xref: https://chromium-review.googlesource.com/c/chromium/src/+/5563251

* Require client for InitParams to always specify an ownership mode.

Xref: https://chromium-review.googlesource.com/c/chromium/src/+/5532482

Xref: https://chromium-review.googlesource.com/c/chromium/src/+/5578714

* chore: e patches all

* fixup! Update namespace for files moved to //components/input

* chore: remove profile_keyed_service_factory, profile_selections from chromium_src

already being linked in via chrome browser for printing

* chore: bump chromium in DEPS to 127.0.6515.0

* chore: bump chromium in DEPS to 127.0.6516.0

* chore: update render_widget_host_view_base.patch

Xref: https://chromium-review.googlesource.com/c/chromium/src/+/5547803

patch applied manually due to simple upstream shear

* chore: update feat_allow_code_cache_in_custom_schemes.patch

No manual changes; patch applied with fuzz 1

* chore: e patches all

* Pull RWHIER and RWT to //content/common/input.

Xref: https://chromium-review.googlesource.com/c/chromium/src/+/5397681

* chore: bump chromium in DEPS to 127.0.6517.0

* chore: update patches

* fixup: Update namespace for files moved to //components/input

* Remove 0-arg (default) constructor for views::Widget::InitParams.

https://chromium-review.googlesource.com/c/chromium/src/+/5578714

* fixup: only disable enterprise_cloud_content_analysis

The original commit a5480ac, was due to this CL 5527572: Move Connectors prefs files to components/enterprise/connectors/ | https://chromium-review.googlesource.com/c/chromium/src/+/5527572

* chore: bump chromium in DEPS to 127.0.6519.0

* chore: update patches

* src: do not use deprecated V8 API

nodejs/node#53084

* src: remove dependency on wrapper-descriptor-based cpp heap

nodejs/node#53086

* 5344413: [DevTools] Add `getHostConfig` UI binding for sending status of `base::Features` to DevTools

https://chromium-review.googlesource.com/c/chromium/src/+/5344413

* 5585788: Extensions: ManifestHandler: Separate Registry like ExtensionRegistry

https://chromium-review.googlesource.com/c/chromium/src/+/5585788

* chore: update filenames.libcxx.gni

* 5506857: Reland "Migrate clang-format to gcs first class deps"

https://chromium-review.googlesource.com/c/chromium/src/+/5506857

* fixup: 5539888: [api] Remove several APIs deprecated in version 12.6

* fixup:  5506857: Reland Migrate clang-format to gcs first class deps

* chore: bump chromium in DEPS to 127.0.6521.0

* chore: update patches

* spec: update navigator.keyboard should lock the keyboard

* Block or allow all MIDI using the existing SysEx permission

Refs https://chromium-review.googlesource.com/c/chromium/src/+/5154368
Refs https://chromium-review.googlesource.com/c/chromium/src/+/5499157

* spec: update test/parallel/test-v8-stats

* views: remove CalculatePreferredSize()

Refs https://chromium-review.googlesource.com/c/chromium/src/+/5504212

* chore: update patches after rebase

* 5560288: Re-enable ChromeOS XNNPack on Intel only

https://chromium-review.googlesource.com/c/chromium/src/+/5560288

* chore: add nan patches for v8 changes

Refs
5539888: [api] Remove several APIs deprecated in version 12.6 | https://chromium-review.googlesource.com/c/v8/v8/+/5539888
and
5539852: [heap][api] Remove deprecated v8::Isolate::IdleNotificationDeadline | https://chromium-review.googlesource.com/c/v8/v8/+/5539852

* 5573603: Modularize //chrome/browser/themes

https://chromium-review.googlesource.com/c/chromium/src/+/5573603

* 5539888: [api] Remove several APIs deprecated in version 12.6

https://chromium-review.googlesource.com/c/v8/v8/+/5539888

* chore: update patches

* test: fixup navigator.keyboard.lock on Windows

* chore: remove unneeded profile target

---------

Co-authored-by: electron-roller[bot] <84116207+electron-roller[bot]@users.noreply.github.com>
Co-authored-by: Keeley Hammond <[email protected]>
Co-authored-by: VerteDinde <[email protected]>
Co-authored-by: Jeremy Rose <[email protected]>
Co-authored-by: clavin <[email protected]>
Co-authored-by: Charles Kerr <[email protected]>
Co-authored-by: PatchUp <73610968+patchup[bot]@users.noreply.github.com>
Co-authored-by: John Kleinschmidt <[email protected]>
Co-authored-by: deepak1556 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnell jasnell jasnell left review comments
@legendecas legendecas legendecas approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs involving general changes in the lib/ or src/ directories. needs-ci PRs that need a full CI run. request-ci-failed Starting CI with the request-ci label failed and requires manual intervention.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

9 participants

@joyeecheung @nodejs-github-bot @targos @addaleax @jasnell @legendecas @StefanStojanovic @fdoray @LeszekSwirski
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.