Implement prerendering cross-origin iframes [chromium/src : main]
Yoichi Osato (Gerrit)
New activity on the change![]( "Open in Gerrit")
Related details
- Hiroki Nakagawa
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hiroki Nakagawa (Gerrit)
Hiroki Nakagawa added 1 comment![]( "Open in Gerrit")
+lingqi@: I'd like to have more reviewers for this new feature. Can you help to review?
Related details
- Hiroki Nakagawa
- Lingqi Chi
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lingqi Chi (Gerrit)
Lingqi Chi added 9 comments![]( "Open in Gerrit")
thank you!
memo) since prerender2 was implemented with the assumption that cross-origin iframes should not loaded until activation, there are some code paths that still hold this assumption. as far as i can tell:
1.
https://source.chromium.org/chromium/chromium/src/+/main:content/browser/renderer_host/render_frame_proxy_host.cc;drc=2b2aabbe4856380ccbeabc14a18600500d13bb46;l=848
Maybe we can leave some TODO comments to track them?
Since it is the basic part, Can we make it behind a feature flag?
// True if cross-origin subframe navigation are allowed.nit/ navigations?
// Allows cross-origin subframes to be prerendered.Maybe `Whether to allow`?
CHECK(page);
if (allow_cross_origin_subframe_navigation_) {
page->render_frame_host()
->GetPage()
.NotifyCrossOriginSubframePrerenderIsAllowed();
}Out of curiosity, are there any reasons that we do not propagate this state when receiving the header? if so, could we leave a brief comment here?
#include "content/browser/preloading/prerender/prerender_subframe_navigation_throttle.h"let's update the class-level comment accordingly
PrerenderSubframeNavigationThrottle::DeferOrCancelCrossOriginSubframeNavigation(maybe we can leave a todo to rename this function? I think it is out-of-date after your CL.
{
event: 'prerendering change',
prerendering: false
},
{
event: 'finished waiting iframe prerender finished',
prerendering: falseI think after this CL, cross-origin iframes can be loaded, is my understanding correct? In this case, it should be possible that "finished waiting iframe prerender finished" fired before the "prerendering change" event?
Related details
- Hiroki Nakagawa
- Yoichi Osato
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hiroki Nakagawa (Gerrit)
Hiroki Nakagawa added 3 comments![]( "Open in Gerrit")
I haven't fully reviewed this yet but let me release early comments.
navigation_handle->GetResponseHeaders();nit: Can we use `navigation_request` for consistency with other parts in this function?
network::ParseSupportsLoadingMode(*headers);Can we use `navigation_request->response()->parsed_headers->supports_loading_mode` like no_vary_search below? Parsing the header in the browser process with C++ is not encouraged.
https://chromium.googlesource.com/chromium/src/+/HEAD/docs/security/rule-of-2.md
Related details
- Yoichi Osato
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lingqi Chi (Gerrit)
Lingqi Chi added 1 comment![]( "Open in Gerrit")
network::ParseSupportsLoadingMode(*headers);Can we use `navigation_request->response()->parsed_headers->supports_loading_mode` like no_vary_search below? Parsing the header in the browser process with C++ is not encouraged.
https://chromium.googlesource.com/chromium/src/+/HEAD/docs/security/rule-of-2.md
TIL!
Yoichi Osato (Gerrit)
Yoichi Osato added 10 comments![]( "Open in Gerrit")
memo) since prerender2 was implemented with the assumption that cross-origin iframes should not loaded until activation, there are some code paths that still hold this assumption. as far as i can tell:
1.
https://source.chromium.org/chromium/chromium/src/+/main:content/browser/renderer_host/render_frame_proxy_host.cc;drc=2b2aabbe4856380ccbeabc14a18600500d13bb46;l=848Maybe we can leave some TODO comments to track them?
#2 is addressed in this CL.
I added a description for #1 in the design doc.
Since it is the basic part, Can we make it behind a feature flag?
Done
// True if cross-origin subframe navigation are allowed.Yoichi Osatonit/ navigations?
Done
Maybe `Whether to allow`?
Done
nit: Can we use `navigation_request` for consistency with other parts in this function?
Done
Lingqi ChiCan we use `navigation_request->response()->parsed_headers->supports_loading_mode` like no_vary_search below? Parsing the header in the browser process with C++ is not encouraged.
https://chromium.googlesource.com/chromium/src/+/HEAD/docs/security/rule-of-2.md
TIL!
Done
CHECK(page);
if (allow_cross_origin_subframe_navigation_) {
page->render_frame_host()
->GetPage()
.NotifyCrossOriginSubframePrerenderIsAllowed();
}Out of curiosity, are there any reasons that we do not propagate this state when receiving the header? if so, could we leave a brief comment here?
I guess PrerenderHost::Activate() activates the Page here and we should not touch Page things before this.
@nhi...@chromium.org for confirmation.
#include "content/browser/preloading/prerender/prerender_subframe_navigation_throttle.h"let's update the class-level comment accordingly
Done
PrerenderSubframeNavigationThrottle::DeferOrCancelCrossOriginSubframeNavigation(maybe we can leave a todo to rename this function? I think it is out-of-date after your CL.
Done
event: 'prerendering change',
prerendering: false
},
{
event: 'finished waiting iframe prerender finished',
prerendering: falseI think after this CL, cross-origin iframes can be loaded, is my understanding correct? In this case, it should be possible that "finished waiting iframe prerender finished" fired before the "prerendering change" event?
You're right. It cannot be determined. Wrote such in the design doc too and updated the test.
Related details
- Hiroki Nakagawa
- Lingqi Chi
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Huanpo Lin (Gerrit)
Huanpo Lin added 1 comment![]( "Open in Gerrit")
ThrottleCheckResult WillStartCrossOriginSubframeNavigation(drive-by comment: Since allowing cross origin subframe still requires opt-in `Supports-Loading-Mode: prerender-cross-origin-frames`, or it will still be deferred, should this be renamed to `MaybeStartCrossOriginSubframeNavigation` or something else?
Related details
- Hiroki Nakagawa
- Lingqi Chi
- Yoichi Osato
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yoichi Osato (Gerrit)
Yoichi Osato added 1 comment![]( "Open in Gerrit")
ThrottleCheckResult WillStartCrossOriginSubframeNavigation(drive-by comment: Since allowing cross origin subframe still requires opt-in `Supports-Loading-Mode: prerender-cross-origin-frames`, or it will still be deferred, should this be renamed to `MaybeStartCrossOriginSubframeNavigation` or something else?
- Hiroki Nakagawa
- Huanpo Lin
- Lingqi Chi
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hiroki Nakagawa (Gerrit)
Hiroki Nakagawa added 6 comments![]( "Open in Gerrit")
Sorry for the late reply. Looks good overall. I'll review tests later.
bool has_no_vary_search_with_parse_error_header = false;
if (navigation_request->response() &&
navigation_request->response()->parsed_headers) {
const network::mojom::ParsedHeadersPtr& parsed_headers =
navigation_request->response()->parsed_headers;
if (parsed_headers->no_vary_search_with_parse_error) {
has_no_vary_search_with_parse_error_header = true;
MaybeSetNoVarySearch(*parsed_headers->no_vary_search_with_parse_error);
}
if (base::FeatureList::IsEnabled(features::kPrerender2CrossOriginIframes) &&
base::Contains(
parsed_headers->supports_loading_mode,
network::mojom::LoadingMode::kPrerenderCrossOriginFrames)) {
allow_cross_origin_subframe_navigation_ = true;
}
}
if (!has_no_vary_search_with_parse_error_header) {
CHECK(!no_vary_search_.has_value());
CHECK(!no_vary_search_parse_error_.has_value());
}`has_no_vary_search_with_parse_error_header` may not be necessary:
```
if (navigation_request->response() &&
navigation_request->response()->parsed_headers) {
const network::mojom::ParsedHeadersPtr& parsed_headers =
navigation_request->response()->parsed_headers;
if (parsed_headers->no_vary_search_with_parse_error) {
has_no_vary_search_with_parse_error_header = true;
MaybeSetNoVarySearch(*parsed_headers->no_vary_search_with_parse_error);
} else {
CHECK(!no_vary_search_.has_value());
CHECK(!no_vary_search_parse_error_.has_value());
}
if (base::FeatureList::IsEnabled(features::kPrerender2CrossOriginIframes) &&
base::Contains(
parsed_headers->supports_loading_mode,
network::mojom::LoadingMode::kPrerenderCrossOriginFrames)) {
allow_cross_origin_subframe_navigation_ = true;
}
}
```
if ((base::FeatureList::IsEnabled(features::kPrerender2CrossOriginIframes)) &&This parentheses is redundant.
CHECK(page);
if (allow_cross_origin_subframe_navigation_) {
page->render_frame_host()
->GetPage()
.NotifyCrossOriginSubframePrerenderIsAllowed();
}Yoichi OsatoOut of curiosity, are there any reasons that we do not propagate this state when receiving the header? if so, could we leave a brief comment here?
I guess PrerenderHost::Activate() activates the Page here and we should not touch Page things before this.
@nhi...@chromium.org for confirmation.
I think there isn't such a restriction.
PrerenderSubframeNavigationThrottle::MaybeStartCrossOriginSubframeNavigation(Actually this function doesn't start or defer the navigation. Instead, this decides the policy for navigation. Thus, how about naming this `DecidePolicyForCrossOriginSubframeNavigation()`?
}It's a bit difficult to follow these conditions in the if-statement. Can we move this outside of the statement, for example, like this?
```
const bool should_send_activation_start = [&]() {
// comments
if (is_main_document) {
return true;
}
// comments
if (base::FeatureList::IsEnabled(
::features::kPrerender2CrossOriginIframes) &&
type == ActivationType::kPrerendering &&
is_cross_origin_subframe_prerender_allowed_) {
return true;
}
// comments
if (type == ActivationType::kPreview) {
return true;
}
return false;
}
if (should_send_activation_start) {
params->activation_start = *activation_start_time_;
}
```
Related details
- Huanpo Lin
- Lingqi Chi
- Yoichi Osato
Yoichi Osato (Gerrit)
Yoichi Osato added 5 comments![]( "Open in Gerrit")
CHECK(!no_vary_XXX) is checked even if `!navigation_request->response()`.
if ((base::FeatureList::IsEnabled(features::kPrerender2CrossOriginIframes)) &&Yoichi OsatoThis parentheses is redundant.
Done
CHECK(page);
if (allow_cross_origin_subframe_navigation_) {
page->render_frame_host()
->GetPage()
.NotifyCrossOriginSubframePrerenderIsAllowed();
}Yoichi OsatoOut of curiosity, are there any reasons that we do not propagate this state when receiving the header? if so, could we leave a brief comment here?
Hiroki NakagawaI guess PrerenderHost::Activate() activates the Page here and we should not touch Page things before this.
@nhi...@chromium.org for confirmation.
I think there isn't such a restriction.
I investigated it and found that the PageImpl is created at RenderFrameHostImpl::DidCommitNavigationInternal() :
https://source.chromium.org/chromium/chromium/src/+/main:content/browser/renderer_host/render_frame_host_impl.cc;drc=36482c6a5e727337ed2676000fca1d095d7c8b3f;l=15597.
That means we don't have it at "PrerenderHost::ReadyToCommitNavigation()".
PrerenderSubframeNavigationThrottle::MaybeStartCrossOriginSubframeNavigation(Actually this function doesn't start or defer the navigation. Instead, this decides the policy for navigation. Thus, how about naming this `DecidePolicyForCrossOriginSubframeNavigation()`?
Done
It's a bit difficult to follow these conditions in the if-statement. Can we move this outside of the statement, for example, like this?
```
const bool should_send_activation_start = [&]() {
// comments
if (is_main_document) {
return true;
}// comments
if (base::FeatureList::IsEnabled(
::features::kPrerender2CrossOriginIframes) &&
type == ActivationType::kPrerendering &&
is_cross_origin_subframe_prerender_allowed_) {
return true;
}// comments
if (type == ActivationType::kPreview) {
return true;
}
return false;
}
if (should_send_activation_start) {
params->activation_start = *activation_start_time_;
}
```
- Hiroki Nakagawa
- Huanpo Lin
- Lingqi Chi
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lingqi Chi (Gerrit)
Lingqi Chi added 7 comments![]( "Open in Gerrit")
if(navigation_request->response) looks like a defensive programming style but I'm wondering if it could be a nullptr when reaching here.
CHECK(page);
if (allow_cross_origin_subframe_navigation_) {
page->render_frame_host()
->GetPage()
.NotifyCrossOriginSubframePrerenderIsAllowed();
}Yoichi OsatoOut of curiosity, are there any reasons that we do not propagate this state when receiving the header? if so, could we leave a brief comment here?
Hiroki NakagawaI guess PrerenderHost::Activate() activates the Page here and we should not touch Page things before this.
@nhi...@chromium.org for confirmation.
Yoichi OsatoI think there isn't such a restriction.
I investigated it and found that the PageImpl is created at RenderFrameHostImpl::DidCommitNavigationInternal() :
https://source.chromium.org/chromium/chromium/src/+/main:content/browser/renderer_host/render_frame_host_impl.cc;drc=36482c6a5e727337ed2676000fca1d095d7c8b3f;l=15597.
That means we don't have it at "PrerenderHost::ReadyToCommitNavigation()".
interesting.. It is my first time to know that during prerendering navigation, the PageImpl will be reset....
reject(`prerendering is wrong. initial_prerendering+${initial_prerendering}, document.prerendering=${document.prerendering}`);nit) state
reject(`prerendering is wrong. initial_prerendering+${initial_prerendering}, document.prerendering=${document.prerendering}`);Does it mean `=`?
if (e.data == 'document.prerendering changes to false from true')how can we ensure that the iframe is loaded before activation? The message is sent by prerenderingchange event listener, so I'm wondering if it is possible that the listener is not registered before activation..?
const prerenderChannel = new PrerenderChannel('prerender-channel');nit) Indentation
Related details
- Hiroki Nakagawa
- Huanpo Lin
- Yoichi Osato
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yoichi Osato (Gerrit)
Yoichi Osato added 4 comments![]( "Open in Gerrit")
reject(`prerendering is wrong. initial_prerendering+${initial_prerendering}, document.prerendering=${document.prerendering}`);Does it mean `=`?
Done
reject(`prerendering is wrong. initial_prerendering+${initial_prerendering}, document.prerendering=${document.prerendering}`);Yoichi Osatonit) state
Done
if (e.data == 'document.prerendering changes to false from true')how can we ensure that the iframe is loaded before activation? The message is sent by prerenderingchange event listener, so I'm wondering if it is possible that the listener is not registered before activation..?
Good catch. I confused prerender/resources/utils.js usage.
Updated the code to confirm prerendering state change before activation via
prerenderChannel.postMessage('readyToActivate').
const prerenderChannel = new PrerenderChannel('prerender-channel');Yoichi Osatonit) Indentation
Done
Related details
- Hiroki Nakagawa
- Huanpo Lin
- Lingqi Chi
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yoichi Osato (Gerrit)
Yoichi Osato voted Commit-Queue+1![]( "Open in Gerrit")
- Hiroki Nakagawa
- Lingqi Chi
Hiroki Nakagawa (Gerrit)
Hiroki Nakagawa added 6 comments![]( "Open in Gerrit")
(I'm still reading test code, but let me release comments before my meeting starts. Feel free to add more reviewers)
CONTENT_EXPORT BASE_DECLARE_FEATURE(kPrerender2CrossOriginIframes);Can you add a link to the crbug?
Thanks! I'm not sure if it's possible... If navigation fils results in an error page, this could be null...? (just wild guess)
Anyway, it's an orthogonal to this change, and it's fine with me to keep this as for now.
// Defer cross-origin subframe navigation until page activation.Can you update this comment? For example,
// Decide a loading policy for cross-origin subframe navigation: cancel, defer it until page activation, or proceed if the opt-in header allows.
This file cannot be upstreamed to WPT until:Actually this test will be upstreamed, as this is under external/wpt 😊
If this is a tentative behavior, can we add the "tentative" filename flag?
https://web-platform-tests.org/writing-tests/file-names.html#test-features
// Since cross-origin iframe is out-of-process, each frames’s activationI wonder if this depends on implementation of browsers. Can we more loosely mention this? For example,
// Since cross iframe can be loaded in a separate renderer process depending on browser implementation, ...
Related details
- Lingqi Chi
- Yoichi Osato
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lingqi Chi (Gerrit)
Lingqi Chi added 2 comments![]( "Open in Gerrit")
thank you!
});iiuc, appendChild does not wait for the child to be fully loaded. so, to ensure the listener is registered before activation, we may need to post a message to ensure the script is loaded.
and then the test looks like:
1. initiator page A starts prerendering B.html
2. adds a iframe(iframe_B), registers a listener for prerenderingchange event(or maybe it is not needed, as it can be verified by smoke tests), a listener for iframe_b's prerenderingchange event message, a listener for iframe_b's loading event message.
3. iframe_B registers a prerenderinghchange event listener (as above)
4. iframe_B informs B that iframe_B is loaded.
5. after receiving iframe_b's loading event message, B asks A to activate B via readyToActivate signal.
6. then b inform the test page of the test success..
Related details
- Yoichi Osato
Hiroki Nakagawa (Gerrit)
Hiroki Nakagawa added 1 comment![]( "Open in Gerrit")
}, (error)=>{
message = error;
})Should this be `catch`?
```
let message;
Promise.all([mainFramePrerenderFinished, iFramePrerenderFinished])
.then(()=>{
message = 'main frame and iframe prerender finished correctly.';
})
.catch((error)=>{
message = error;
})
.finally(()=>{
// Send the observed events back to the main test page.
const testChannel = new PrerenderChannel('test-channel');
testChannel.postMessage(message);
testChannel.close();
});
```
Related details
- Yoichi Osato
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yoichi Osato (Gerrit)
Yoichi Osato added 7 comments![]( "Open in Gerrit")
(I'm still reading test code, but let me release comments before my meeting starts. Feel free to add more reviewers)
I think it is good to get LGTM among the team before asking other OWNERs review.
CONTENT_EXPORT BASE_DECLARE_FEATURE(kPrerender2CrossOriginIframes);Can you add a link to the crbug?
Done
// Defer cross-origin subframe navigation until page activation.Can you update this comment? For example,
// Decide a loading policy for cross-origin subframe navigation: cancel, defer it until page activation, or proceed if the opt-in header allows.
Done
Actually this test will be upstreamed, as this is under external/wpt 😊
If this is a tentative behavior, can we add the "tentative" filename flag?
https://web-platform-tests.org/writing-tests/file-names.html#test-features
Based on we're updating the spec, just let me upstream this to the public WPT.
// Since cross-origin iframe is out-of-process, each frames’s activationI wonder if this depends on implementation of browsers. Can we more loosely mention this? For example,
// Since cross iframe can be loaded in a separate renderer process depending on browser implementation, ...
Ah I didn't know that depends the impls.
}, (error)=>{
message = error;
})Should this be `catch`?
```
let message;
Promise.all([mainFramePrerenderFinished, iFramePrerenderFinished])
.then(()=>{
message = 'main frame and iframe prerender finished correctly.';
})
.catch((error)=>{
message = error;
})
.finally(()=>{
// Send the observed events back to the main test page.
const testChannel = new PrerenderChannel('test-channel');
testChannel.postMessage(message);
testChannel.close();
});
```
.then(res,rej) is same as .then(res).catch(rej).
iiuc, appendChild does not wait for the child to be fully loaded. so, to ensure the listener is registered before activation, we may need to post a message to ensure the script is loaded.
and then the test looks like:
1. initiator page A starts prerendering B.html
2. adds a iframe(iframe_B), registers a listener for prerenderingchange event(or maybe it is not needed, as it can be verified by smoke tests), a listener for iframe_b's prerenderingchange event message, a listener for iframe_b's loading event message.
3. iframe_B registers a prerenderinghchange event listener (as above)
4. iframe_B informs B that iframe_B is loaded.
5. after receiving iframe_b's loading event message, B asks A to activate B via readyToActivate signal.
6. then b inform the test page of the test success..
Waiting each document.prerendering to false before activation is not enough?
Related details
- Hiroki Nakagawa
- Lingqi Chi
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lingqi Chi (Gerrit)
Lingqi Chi added 2 comments![]( "Open in Gerrit")
thank you!
Yoichi Osatoiiuc, appendChild does not wait for the child to be fully loaded. so, to ensure the listener is registered before activation, we may need to post a message to ensure the script is loaded.
and then the test looks like:
1. initiator page A starts prerendering B.html
2. adds a iframe(iframe_B), registers a listener for prerenderingchange event(or maybe it is not needed, as it can be verified by smoke tests), a listener for iframe_b's prerenderingchange event message, a listener for iframe_b's loading event message.
3. iframe_B registers a prerenderinghchange event listener (as above)
4. iframe_B informs B that iframe_B is loaded.
5. after receiving iframe_b's loading event message, B asks A to activate B via readyToActivate signal.
6. then b inform the test page of the test success..
Waiting each document.prerendering to false before activation is not enough?
I think it is enough in terms of correctness, just wondering if the test would be flaky on some bots because the listener may not be registered when activation..
Related details
- Hiroki Nakagawa
- Yoichi Osato
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hiroki Nakagawa (Gerrit)
Hiroki Nakagawa voted and added 2 comments![]( "Open in Gerrit")
Votes added by Hiroki Nakagawa
| Code-Review | +1 |
2 comments
LGTM, thanks! I'll delegate reviews for test stability to lingqi@.
}, (error)=>{
message = error;
})Yoichi OsatoShould this be `catch`?
```
let message;
Promise.all([mainFramePrerenderFinished, iFramePrerenderFinished])
.then(()=>{
message = 'main frame and iframe prerender finished correctly.';
})
.catch((error)=>{
message = error;
})
.finally(()=>{
// Send the observed events back to the main test page.
const testChannel = new PrerenderChannel('test-channel');
testChannel.postMessage(message);
testChannel.close();
});
```
.then(res,rej) is same as .then(res).catch(rej).
- Yoichi Osato
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yoichi Osato (Gerrit)
Yoichi Osato added 1 comment![]( "Open in Gerrit")
Yoichi Osatoiiuc, appendChild does not wait for the child to be fully loaded. so, to ensure the listener is registered before activation, we may need to post a message to ensure the script is loaded.
and then the test looks like:
1. initiator page A starts prerendering B.html
2. adds a iframe(iframe_B), registers a listener for prerenderingchange event(or maybe it is not needed, as it can be verified by smoke tests), a listener for iframe_b's prerenderingchange event message, a listener for iframe_b's loading event message.
3. iframe_B registers a prerenderinghchange event listener (as above)
4. iframe_B informs B that iframe_B is loaded.
5. after receiving iframe_b's loading event message, B asks A to activate B via readyToActivate signal.
6. then b inform the test page of the test success..
Lingqi ChiWaiting each document.prerendering to false before activation is not enough?
I think it is enough in terms of correctness, just wondering if the test would be flaky on some bots because the listener may not be registered when activation..
We await iFramePrerenderFinished at L49 before L65 postMessage('readyToActivate').
Awaiting iFramePrerenderFinished means
```
"await" document.addEventListener('prerenderingchange') (fully loaded)
"await" window.parent.postMessage(`document.prerendering...
"await" if (e.data == 'document.prerendering chan...
resolve(); (in page B)
```
So L65:postMessage('readyToActivate') is never called w/o iframes event registration and callback, as I understand it.Related details
- Lingqi Chi
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lingqi Chi (Gerrit)
Lingqi Chi added 2 comments![]( "Open in Gerrit")
thank you!
Yoichi Osatoiiuc, appendChild does not wait for the child to be fully loaded. so, to ensure the listener is registered before activation, we may need to post a message to ensure the script is loaded.
and then the test looks like:
1. initiator page A starts prerendering B.html
2. adds a iframe(iframe_B), registers a listener for prerenderingchange event(or maybe it is not needed, as it can be verified by smoke tests), a listener for iframe_b's prerenderingchange event message, a listener for iframe_b's loading event message.
3. iframe_B registers a prerenderinghchange event listener (as above)
4. iframe_B informs B that iframe_B is loaded.
5. after receiving iframe_b's loading event message, B asks A to activate B via readyToActivate signal.
6. then b inform the test page of the test success..
Lingqi ChiWaiting each document.prerendering to false before activation is not enough?
Yoichi OsatoI think it is enough in terms of correctness, just wondering if the test would be flaky on some bots because the listener may not be registered when activation..
We await iFramePrerenderFinished at L49 before L65 postMessage('readyToActivate').
Awaiting iFramePrerenderFinished means
```
"await" document.addEventListener('prerenderingchange') (fully loaded)
"await" window.parent.postMessage(`document.prerendering...
"await" if (e.data == 'document.prerendering chan...
resolve(); (in page B)
```
So L65:postMessage('readyToActivate') is never called w/o iframes event registration and callback, as I understand it.IIUC, Promise.proto.then() and Promise.proto.finally() return a Promise, without waiting for it to be resolved.
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise/then
Related details
- Yoichi Osato
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yoichi Osato (Gerrit)
Yoichi Osato added 1 comment![]( "Open in Gerrit")
Yoichi Osatoiiuc, appendChild does not wait for the child to be fully loaded. so, to ensure the listener is registered before activation, we may need to post a message to ensure the script is loaded.
and then the test looks like:
1. initiator page A starts prerendering B.html
2. adds a iframe(iframe_B), registers a listener for prerenderingchange event(or maybe it is not needed, as it can be verified by smoke tests), a listener for iframe_b's prerenderingchange event message, a listener for iframe_b's loading event message.
3. iframe_B registers a prerenderinghchange event listener (as above)
4. iframe_B informs B that iframe_B is loaded.
5. after receiving iframe_b's loading event message, B asks A to activate B via readyToActivate signal.
6. then b inform the test page of the test success..
Lingqi ChiWaiting each document.prerendering to false before activation is not enough?
Yoichi OsatoI think it is enough in terms of correctness, just wondering if the test would be flaky on some bots because the listener may not be registered when activation..
We await iFramePrerenderFinished at L49 before L65 postMessage('readyToActivate').
Awaiting iFramePrerenderFinished means
```
"await" document.addEventListener('prerenderingchange') (fully loaded)
"await" window.parent.postMessage(`document.prerendering...
"await" if (e.data == 'document.prerendering chan...
resolve(); (in page B)
```
So L65:postMessage('readyToActivate') is never called w/o iframes event registration and callback, as I understand it.IIUC, Promise.proto.then() and Promise.proto.finally() return a Promise, without waiting for it to be resolved.
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise/then
- Lingqi Chi
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lingqi Chi (Gerrit)
Lingqi Chi voted and added 1 comment![]( "Open in Gerrit")
Votes added by Lingqi Chi
| Code-Review | +1 |
thank you!
- Yoichi Osato
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yoichi Osato (Gerrit)
Yoichi Osato added 2 comments![]( "Open in Gerrit")
bool has_no_vary_search_with_parse_error_header = false;
if (navigation_request->response() &&
navigation_request->response()->parsed_headers) {
const network::mojom::ParsedHeadersPtr& parsed_headers =
navigation_request->response()->parsed_headers;
if (parsed_headers->no_vary_search_with_parse_error) {
has_no_vary_search_with_parse_error_header = true;
MaybeSetNoVarySearch(*parsed_headers->no_vary_search_with_parse_error);
}
if (base::FeatureList::IsEnabled(features::kPrerender2CrossOriginIframes) &&
base::Contains(
parsed_headers->supports_loading_mode,
network::mojom::LoadingMode::kPrerenderCrossOriginFrames)) {
allow_cross_origin_subframe_navigation_ = true;
}
}
if (!has_no_vary_search_with_parse_error_header) {
CHECK(!no_vary_search_.has_value());
CHECK(!no_vary_search_parse_error_.has_value());
}Yoichi Osato`has_no_vary_search_with_parse_error_header` may not be necessary:
```
if (navigation_request->response() &&
navigation_request->response()->parsed_headers) {
const network::mojom::ParsedHeadersPtr& parsed_headers =
navigation_request->response()->parsed_headers;
if (parsed_headers->no_vary_search_with_parse_error) {
has_no_vary_search_with_parse_error_header = true;
MaybeSetNoVarySearch(*parsed_headers->no_vary_search_with_parse_error);
} else {
CHECK(!no_vary_search_.has_value());
CHECK(!no_vary_search_parse_error_.has_value());
}if (base::FeatureList::IsEnabled(features::kPrerender2CrossOriginIframes) &&
base::Contains(
parsed_headers->supports_loading_mode,
network::mojom::LoadingMode::kPrerenderCrossOriginFrames)) {
allow_cross_origin_subframe_navigation_ = true;
}
}
```
Lingqi ChiCHECK(!no_vary_XXX) is checked even if `!navigation_request->response()`.
Hiroki Nakagawaif(navigation_request->response) looks like a defensive programming style but I'm wondering if it could be a nullptr when reaching here.
Thanks! I'm not sure if it's possible... If navigation fils results in an error page, this could be null...? (just wild guess)
Anyway, it's an orthogonal to this change, and it's fine with me to keep this as for now.
Let me resolved this so far.
CHECK(page);
if (allow_cross_origin_subframe_navigation_) {
page->render_frame_host()
->GetPage()
.NotifyCrossOriginSubframePrerenderIsAllowed();
}Yoichi OsatoOut of curiosity, are there any reasons that we do not propagate this state when receiving the header? if so, could we leave a brief comment here?
Hiroki NakagawaI guess PrerenderHost::Activate() activates the Page here and we should not touch Page things before this.
@nhi...@chromium.org for confirmation.
Yoichi OsatoI think there isn't such a restriction.
Lingqi ChiI investigated it and found that the PageImpl is created at RenderFrameHostImpl::DidCommitNavigationInternal() :
https://source.chromium.org/chromium/chromium/src/+/main:content/browser/renderer_host/render_frame_host_impl.cc;drc=36482c6a5e727337ed2676000fca1d095d7c8b3f;l=15597.
That means we don't have it at "PrerenderHost::ReadyToCommitNavigation()".
interesting.. It is my first time to know that during prerendering navigation, the PageImpl will be reset....
- Takashi Toyoshima
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yoichi Osato (Gerrit)
Yoichi Osato added 1 comment![]( "Open in Gerrit")
Yoichi Osato(I'm still reading test code, but let me release comments before my meeting starts. Feel free to add more reviewers)
I think it is good to get LGTM among the team before asking other OWNERs review.
- Koji Ishii
- Rakina Zata Amni
- Takashi Toyoshima
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yoichi Osato (Gerrit)
Yoichi Osato added 1 comment![]( "Open in Gerrit")
Ask
toyoshim@ for /services review.
tkent@ for VirtualTest review.
PTAL.
Related details
- Kent Tamura
- Takashi Toyoshima
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kent Tamura (Gerrit)
Kent Tamura voted and added 1 comment![]( "Open in Gerrit")
Votes added by Kent Tamura
| Code-Review | +1 |
1 comment
- Takashi Toyoshima
- Yoichi Osato
Yoichi Osato (Gerrit)
Yoichi Osato added 1 comment![]( "Open in Gerrit")
Ask jonross@ for c/b/r/page_impl.* review. PTAL.
Related details
- Jonathan Ross
- Takashi Toyoshima
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Takashi Toyoshima (Gerrit)
Takashi Toyoshima voted and added 1 comment![]( "Open in Gerrit")
Votes added by Takashi Toyoshima
| Code-Review | +1 |
1 comment
if (base::FeatureList::IsEnabled(features::kPrerender2CrossOriginIframes) &&Maybe
```
if (prerender_host->AllowCrossOriginSubframeNavigation()) {
CHECK(base::FeatureList::IsEnabled(...));
return NavigationThrottle::PROCEED;
}
```
If we follow the existing check pattern around the prerender related base::Features, the feature is gated at an earlier place, PrerenderHost, and internal code just respects the PrerenderHost's decision and does CHECK(IsEnabled()).
Related details
- Jonathan Ross
- Yoichi Osato
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yoichi Osato (Gerrit)
Yoichi Osato added 1 comment![]( "Open in Gerrit")
if (base::FeatureList::IsEnabled(features::kPrerender2CrossOriginIframes) &&Maybe
```
if (prerender_host->AllowCrossOriginSubframeNavigation()) {
CHECK(base::FeatureList::IsEnabled(...));
return NavigationThrottle::PROCEED;
}
```
If we follow the existing check pattern around the prerender related base::Features, the feature is gated at an earlier place, PrerenderHost, and internal code just respects the PrerenderHost's decision and does CHECK(IsEnabled()).
Done
Related details
- Jonathan Ross
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jonathan Ross (Gerrit)
Jonathan Ross voted Code-Review+1![]( "Open in Gerrit")
- Yoichi Osato
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Blink W3C Test Autoroller (Gerrit)
Message from Blink W3C Test Autoroller![]( "Open in Gerrit")
Exportable changes to web-platform-tests were detected in this CL and a pull request in the upstream repo has been made: https://github.com/web-platform-tests/wpt/pull/54750.
When this CL lands, the bot will automatically merge the PR on GitHub if the required GitHub checks pass; otherwise, ecosystem-infra@ team will triage the failures and may contact you.
WPT Export docs:
https://chromium.googlesource.com/chromium/src/+/main/docs/testing/web_platform_tests.md#Automatic-export-process
Related details
- Yoichi Osato
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |