Andreas Haas (Gerrit)

unread,

Aug 7, 2025, 9:03:04 AMAug 7

to Omer Katz, Kentaro Hara, AyeAye, Chromium LUCI CQ, kinuko...@chromium.org, kouhe...@chromium.org, oilpan-rev...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org

Attention needed from Kentaro Hara and Omer Katz

Andreas Haas added 2 comments

Patchset-level comments

File-level comment, Patchset 8 (Latest):

Andreas Haas . resolved

Omer, can you please do an expert review?
Kentaro, can you please do an owner review?

File chrome/renderer/supervised_user/supervised_user_error_page_controller.cc

Line 67, Patchset 1: static_cast<blink::Visitor*>(visitor)->Trace(weak_factory_);

Omer Katz . resolved

Why do you need this cast? `blink::Visitor` should be an alias of `cppgc::Visitor`.

Andreas Haas

Done

Open in Gerrit

Related details

Attention is currently required from:

Kentaro Hara
Omer Katz

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Omer Katz (Gerrit)

unread,

Aug 7, 2025, 9:35:55 AMAug 7

to Andreas Haas, Kentaro Hara, AyeAye, Chromium LUCI CQ, kinuko...@chromium.org, kouhe...@chromium.org, oilpan-rev...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org

Attention needed from Andreas Haas and Kentaro Hara

Omer Katz voted and added 1 comment

Votes added by Omer Katz

Code-Review

+1

1 comment

Patchset-level comments

File-level comment, Patchset 9 (Latest):

Omer Katz . resolved

lgtm

Open in Gerrit

Related details

Attention is currently required from:

Andreas Haas
Kentaro Hara

Submit Requirements:

Code-Coverage
Code-Owners

Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Kentaro Hara (Gerrit)

unread,

Aug 7, 2025, 8:09:33 PMAug 7

to Andreas Haas, Omer Katz, AyeAye, Chromium LUCI CQ, kinuko...@chromium.org, kouhe...@chromium.org, oilpan-rev...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org

Attention needed from Andreas Haas

Kentaro Hara voted and added 1 comment

Votes added by Kentaro Hara

Code-Review

+1

1 comment

Patchset-level comments

File-level comment, Patchset 9 (Latest):

Kentaro Hara . resolved

LGTM

Open in Gerrit

Related details

Attention is currently required from:

Andreas Haas

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Andreas Haas (Gerrit)

unread,

Aug 8, 2025, 12:31:41 AMAug 8

to Georg Neis, Kentaro Hara, Omer Katz, AyeAye, Chromium LUCI CQ, kinuko...@chromium.org, kouhe...@chromium.org, oilpan-rev...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org

Attention needed from Georg Neis

Andreas Haas added 1 comment

Patchset-level comments

File-level comment, Patchset 9 (Latest):

Andreas Haas . resolved

Georg, can you please do an owner review of chrome/*

Open in Gerrit

Related details

Attention is currently required from:

Georg Neis

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Georg Neis (Gerrit)

unread,

Aug 8, 2025, 12:37:07 AMAug 8

to Andreas Haas, Kentaro Hara, Omer Katz, AyeAye, Chromium LUCI CQ, kinuko...@chromium.org, kouhe...@chromium.org, oilpan-rev...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org

Attention needed from Andreas Haas

Georg Neis added 1 comment

Patchset-level comments

File-level comment, Patchset 9 (Latest):

Georg Neis . resolved

Sorry, I'm owner only for ash refactoring related stuff. Assiging to supervised_user owners.

Open in Gerrit

Related details

Attention is currently required from:

Andreas Haas

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

gwsq (Gerrit)

unread,

Aug 8, 2025, 12:39:48 AMAug 8

to Andreas Haas, chrome-family...@google.com, Tomek Jurkiewicz, Kentaro Hara, Omer Katz, AyeAye, Chromium LUCI CQ, kinuko...@chromium.org, kouhe...@chromium.org, oilpan-rev...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org

Attention needed from Andreas Haas and Tomek Jurkiewicz

Message from gwsq

Reviewer source(s):
t...@google.com is from context(googleclient/chrome/chromium_gwsq/chrome/browser/supervised_user/config.gwsq)

Open in Gerrit

Related details

Attention is currently required from:

Andreas Haas
Tomek Jurkiewicz

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Tomek Jurkiewicz (Gerrit)

unread,

Aug 8, 2025, 4:02:04 AMAug 8

to Andreas Haas, chrome-family...@google.com, Kentaro Hara, Omer Katz, AyeAye, Chromium LUCI CQ, kinuko...@chromium.org, kouhe...@chromium.org, oilpan-rev...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org

Attention needed from Andreas Haas

Tomek Jurkiewicz added 1 comment

Patchset-level comments

File-level comment, Patchset 10 (Latest):

Tomek Jurkiewicz . resolved

Hey,

Can you be a more specific, why c/r/supervised_user is elected to use weak cell? This change looks like it's introducing weak cell, and immediately makes only one specific error page to use it.

Open in Gerrit

Related details

Attention is currently required from:

Andreas Haas

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Andreas Haas (Gerrit)

unread,

Aug 8, 2025, 4:33:53 AMAug 8

to chrome-family...@google.com, Tomek Jurkiewicz, Kentaro Hara, Omer Katz, AyeAye, Chromium LUCI CQ, kinuko...@chromium.org, kouhe...@chromium.org, oilpan-rev...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org

Attention needed from Tomek Jurkiewicz

Andreas Haas added 1 comment

Patchset-level comments

File-level comment, Patchset 10 (Latest):

Tomek Jurkiewicz . resolved

Hey,
Can you be a more specific, why c/r/supervised_user is elected to use weak cell? This change looks like it's introducing weak cell, and immediately makes only one specific error page to use it.

Andreas Haas

In https://crrev.com/c/6703729, `SupervisedUserErrorPageController` was migrated to a more secure implementation of `gin::Wrappable`. As part of this migration, `SupervisedUserErrorPageController` became an oilpan object, i.e. its memory is managed by a GC. Due to that, `base::WeakPtrFactory` is not allowed to be used anymore. This change should have already been done as part of the original migration, but it was missed.

`gin::WeakCellFactory` is the Oilpan equivalent to `base::WeakPtrFactory`.

Open in Gerrit

Related details

Attention is currently required from:

Tomek Jurkiewicz

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Tomek Jurkiewicz (Gerrit)

unread,

Aug 8, 2025, 4:48:27 AMAug 8

to Andreas Haas, chrome-family...@google.com, Kentaro Hara, Omer Katz, AyeAye, Chromium LUCI CQ, kinuko...@chromium.org, kouhe...@chromium.org, oilpan-rev...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org

Attention needed from Andreas Haas

Tomek Jurkiewicz added 1 comment

Patchset-level comments

File-level comment, Patchset 10 (Latest):

Tomek Jurkiewicz . unresolved

Hey,
Can you be a more specific, why c/r/supervised_user is elected to use weak cell? This change looks like it's introducing weak cell, and immediately makes only one specific error page to use it.

Andreas Haas

In https://crrev.com/c/6703729, `SupervisedUserErrorPageController` was migrated to a more secure implementation of `gin::Wrappable`. As part of this migration, `SupervisedUserErrorPageController` became an oilpan object, i.e. its memory is managed by a GC. Due to that, `base::WeakPtrFactory` is not allowed to be used anymore. This change should have already been done as part of the original migration, but it was missed.
`gin::WeakCellFactory` is the Oilpan equivalent to `base::WeakPtrFactory`.

Tomek Jurkiewicz

I see, thanks.

But I think my argument is still valid; that's two changes in one CL. Do you think you could split out infra change from supervised user change?

Open in Gerrit

Related details

Attention is currently required from:

Andreas Haas

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review

No-Unresolved-Comments

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Reply all

Reply to author

Forward

Make `SupervisedUserErrorPageController` use `WeakCellFactory` [chromium/src : main]

Andreas Haas (Gerrit)

Andreas Haas added 2 comments

Related details

Omer Katz (Gerrit)

Omer Katz voted and added 1 comment

Votes added by Omer Katz

1 comment

Related details

Kentaro Hara (Gerrit)

Kentaro Hara voted and added 1 comment

Votes added by Kentaro Hara

1 comment

Related details

Andreas Haas (Gerrit)

Andreas Haas added 1 comment

Related details

Georg Neis (Gerrit)

Georg Neis added 1 comment

Related details

gwsq (Gerrit)

Message from gwsq

Related details

Tomek Jurkiewicz (Gerrit)

Tomek Jurkiewicz added 1 comment

Related details

Andreas Haas (Gerrit)

Andreas Haas added 1 comment

Related details

Tomek Jurkiewicz (Gerrit)

Tomek Jurkiewicz added 1 comment

Related details