Prevent origin updates in same-document navigations and reclassify MHTML navigations [chromium/src : main]
Diana Qu (Gerrit)
Diana Qu added 6 comments![]( "Open in Gerrit")
FYI I update the title to affect more closely to the change in this CL
if (!was_within_same_document) {Diana QuSince there's a risk we could end up with issues from either the MHTML change or the new renderer kills, I would suggest including a base::Feature killswitch that allows us to turn the changes off if needed (e.g., on Stable if that's when we notice it). That could guard each of the 3 functional changes. (I'm not opposed to using separate features for the content/ and blink/ changes if that's easier.)
Updated.
}Diana QuIn Alex's https://chromium-review.googlesource.com/c/chromium/src/+/1081937/3/content/browser/frame_host/navigator_impl.cc, he tried out an else block with CHECKs to verify that same-document navigations weren't changing these values in practice. Apparently that failed a lot of tests at the time (2018), as listed on https://crbug.com/40580002.
Can you run try jobs with similar CHECKs in this CL, to see if those issues have been resolved already or still happen? We may want to get to the bottom of it if they're still failing.
Diana QuALL passing for this case.
Charlie ReisDo we want to end up keeping both two places that CHECK on origins? Seems like its good to keep to catch bugs.
Diana QuI like the idea of keeping some kind of validation. We don't want to let the renderer kill the browser process by sending bad parameters, but there's ways to handle that. For example, we could do a renderer kill when validating the parameters if the insecure_request_policy or insecure_navigations_set differ on a same document navigation. That would need to happen earlier, such as with some of the other renderer kills in RenderFrameHostImpl::DidCommitNavigationInternal (which calls this function). After that, it should be impossible for the renderer to make us fail the CHECK with a bad parameter, so we could keep the CHECK here as documentation and a backstop, or remove it as redundant.
(In all of these cases, we may want to use the NotFatalUntil or killswitch approach to be able to react in case there are unexpected failures in the wild, which we might not notice until stable channel.)
Charlie ReisLet me think on what to do here. I feel like we shouldn't crash browser in same document mismatch case in real life, but want to keep some kind of signal to help catch bugs
Diana QuIn the short term, DumpWithoutCrashing gives a signal without actually crashing the browser. I think a renderer kill isn't a bad option, since that also creates a crash report without taking down the browser process, and only prevents further corruption from a misbehaving (either buggy or compromised) renderer. This will matter more as RenderDocument makes the origin of a RenderFrameHost const, and hopefully things like these values as well.
Diana QuSure. I will do just a renderer crash in DidCommitNavigationInternal instead.
Charlie ReisUpdated. with renderer kill and validated that without mhtml change the test will fail.
Diana QuThanks! Which test failure are you referring to? Maybe we can mention the test name in the CL description.
Sure. Let me add that to the description.
GetProcess(), bad_message::RFH_INVALID_ORIGIN_ON_COMMIT);Diana QuAh, it's best not to reuse existing renderer kill codes, because that makes it harder to tell which one is occurring when we look at histograms. Let's use a new unique value here.
It's probably also worth referencing same-document in all of these codes, along the lines of:
RFH_SAME_DOC_ORIGIN_CHANGE
RFH_SAME_DOC_INSECURE_REQUEST_POLICY_CHANGE
RFH_SAME_DOC_INSECURE_NAV_SET_CHANGE
Updated.
was_discarded_ = navigation_request->commit_params().was_discarded;Diana QuI think this is a ClangTidy's false alert, but anyway you need a following line in the CL description to skip the check.
```
Skip-Clang-Tidy-Checks: bugprone-use-after-move
```
Diana QuI see. I run the git cl format tools and end up with these change. I can revert this style change.
Diana QuAh nvm. I will add to CL description. Thanks!
Done
same_document_navigation = false;Diana QuI realized there's a problem with this approach, though I'm curious for dcheng@'s or lukasza@'s take on whether/how to fix it.
Here, we're unconditionally classifying all navigations within MHTML pages be cross-document, to handle the case where about:blank#foo in MHTML is actually cross-document (and thus changes origins).
Unfortunately, it is possible to have actual same-document navigations in MHTML, though apparently another bug makes that a little tricky. Imagine saving a page with a #foo link to MHTML (such as https://csreis.github.io/tests/same-document-navigations.html), loading the archive, and then clicking on the link. That should be same-document, but then this code would force it to be cross-document (and worse, it would result in an empty document because that #foo URL is not saved in the archive).
I've tried this in practice (including within a subframe) and there's a separate bug where the #foo link gets rewritten to include the full URL (https://csreis.github.io/tests/same-document-navigations.html#foo), and thus it doesn't look like a same-document URL from the MHTML's version of the URL. This means the #foo link is already basically broken today.
Using DevTools, I found it was technically still possible to navigate to "#foo" and get a same-document navigation, and I confirmed that this new code made it cross-document (and thus broken again). Without DevTools, I'm not sure if we can run into this problem, since MHTML can't run scripts (e.g., pushState), and all same-document links appear to be rewritten to leave the archive.
I'm not quite sure how we should approach this. Long term, it seems like we should fix the MHTML bug and allow same-document navigations, which would mean finding a different way to handle this case (e.g., only treating that first about:blank#foo navigation during archive loading as cross-document, but not all navigations within MHTML documents?). For now, it appears really hard to get a same-document navigation in MHTML to begin with (without opening DevTools), so maybe a TODO in this block isn't out of the question?
Doesn't sounds like classify MTHML scope to only #framgent to cross-document is doable right now. I added some todos here just in case we decided to ship first and fix the #foo case later. (https://issues.chromium.org/issues/423663315). But I'm open with discussions.
Related details
- Charlie Reis
- Daniel Cheng
- Takashi Toyoshima
- Łukasz Anforowicz
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Łukasz Anforowicz (Gerrit)
Łukasz Anforowicz added 3 comments![]( "Open in Gerrit")
- As a result, NavigationMhtmlBrowserTest.IframeAboutBlankNotFound
now passes, since the navigation is no longer misclassified.Can you help me when/how the test becomes broken? It's not broken today, so I assume that the test will break after step 1 (enforcing invariants on same-document navigations)?
was_discarded_ = navigation_request->commit_params().was_discarded;Please fix this WARNING reported by ClangTidy: check: bugprone-use-after-move
Analyzer Description: Runs clang-tidy checks against chromium
Owner: gb...@chromium.org
check: bugprone-use-after-move
'navigation_request' used after it was moved (https://clang.llvm.org/extra/clang-tidy/checks/bugprone/use-after-move.html)
(Note: You can add `Skip-Clang-Tidy-Checks: bugprone-use-after-move` footer to the CL description to skip the check)
(Lint observed on `android-clang-tidy-rel` and `linux-clang-tidy-rel`)
same_document_navigation = false;Diana QuI realized there's a problem with this approach, though I'm curious for dcheng@'s or lukasza@'s take on whether/how to fix it.
Here, we're unconditionally classifying all navigations within MHTML pages be cross-document, to handle the case where about:blank#foo in MHTML is actually cross-document (and thus changes origins).
Unfortunately, it is possible to have actual same-document navigations in MHTML, though apparently another bug makes that a little tricky. Imagine saving a page with a #foo link to MHTML (such as https://csreis.github.io/tests/same-document-navigations.html), loading the archive, and then clicking on the link. That should be same-document, but then this code would force it to be cross-document (and worse, it would result in an empty document because that #foo URL is not saved in the archive).
I've tried this in practice (including within a subframe) and there's a separate bug where the #foo link gets rewritten to include the full URL (https://csreis.github.io/tests/same-document-navigations.html#foo), and thus it doesn't look like a same-document URL from the MHTML's version of the URL. This means the #foo link is already basically broken today.
Using DevTools, I found it was technically still possible to navigate to "#foo" and get a same-document navigation, and I confirmed that this new code made it cross-document (and thus broken again). Without DevTools, I'm not sure if we can run into this problem, since MHTML can't run scripts (e.g., pushState), and all same-document links appear to be rewritten to leave the archive.
I'm not quite sure how we should approach this. Long term, it seems like we should fix the MHTML bug and allow same-document navigations, which would mean finding a different way to handle this case (e.g., only treating that first about:blank#foo navigation during archive loading as cross-document, but not all navigations within MHTML documents?). For now, it appears really hard to get a same-document navigation in MHTML to begin with (without opening DevTools), so maybe a TODO in this block isn't out of the question?
Doesn't sounds like classify MTHML scope to only #framgent to cross-document is doable right now. I added some todos here just in case we decided to ship first and fix the #foo case later. (https://issues.chromium.org/issues/423663315). But I'm open with discussions.
I've tried this in practice (including within a subframe) and there's a separate bug where the #foo link gets rewritten to include the full URL (https://csreis.github.io/tests/same-document-navigations.html#foo), and thus it doesn't look like a same-document URL from the MHTML's version of the URL. This means the #foo link is already basically broken today.
What kind of broken-ness are we talking about?
- Classifying the navigation as same-document VS cross-document?
- Navigation going to the network, instead of staying within the MHTML archive?
If we're talking about the latter, then maybe we should (in a follow-up CL?) change how URLs are looked up in MHTML archive (discarding `#foo` from URLs before using them as a key). That said, I haven't looked at how the code actually does MHTML archive entry lookup, so maybe what I said here is inaccurate / misleading / wrong...
Related details
- Charlie Reis
- Daniel Cheng
- Diana Qu
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Diana Qu (Gerrit)
Diana Qu added 2 comments![]( "Open in Gerrit")
- As a result, NavigationMhtmlBrowserTest.IframeAboutBlankNotFound
now passes, since the navigation is no longer misclassified.Can you help me when/how the test becomes broken? It's not broken today, so I assume that the test will break after step 1 (enforcing invariants on same-document navigations)?
Yes, it will break after step 1. because MHTML is classifying as same document navigation but in fact new document is committed and new opaque origins are created. (I can update the description with more details if that helps)
was_discarded_ = navigation_request->commit_params().was_discarded;Please fix this WARNING reported by ClangTidy: check: bugprone-use-after-move
Analyzer Description: Runs clang-tidy checks against chromium
Owner: gb...@chromium.orgcheck: bugprone-use-after-move
'navigation_request' used after it was moved (https://clang.llvm.org/extra/clang-tidy/checks/bugprone/use-after-move.html)
(Note: You can add `Skip-Clang-Tidy-Checks: bugprone-use-after-move` footer to the CL description to skip the check)
(Lint observed on `android-clang-tidy-rel` and `linux-clang-tidy-rel`)
I can just revert this line locally. It got added after running `git cl format`
Related details
- Charlie Reis
- Daniel Cheng
- Łukasz Anforowicz
Diana Qu (Gerrit)
Diana Qu added 1 comment![]( "Open in Gerrit")
GetProcess(), bad_message::RFH_INVALID_ORIGIN_ON_COMMIT);Diana QuAh, it's best not to reuse existing renderer kill codes, because that makes it harder to tell which one is occurring when we look at histograms. Let's use a new unique value here.
It's probably also worth referencing same-document in all of these codes, along the lines of:
RFH_SAME_DOC_ORIGIN_CHANGE
RFH_SAME_DOC_INSECURE_REQUEST_POLICY_CHANGE
RFH_SAME_DOC_INSECURE_NAV_SET_CHANGE
Updated.
I noticed that `CrossOriginSameDocumentCommit` and `CrossOriginSameDocumentCommitFromAboutBlank` fail after I change the error message code. Both this test expect `RFH_INVALID_ORIGIN_ON_COMMIT`, but since we do the origin check early now, it will return `RFH_SAME_DOC_ORIGIN_CHANGE`.
I prefer to just remove the origin check in this mismatch case since we are verifying origins in `CanCommitOriginAndUrl` later down the line and have a error message corresponding to it already. (Since it seems like we don't want to reuse these code?)
Related details
- Charlie Reis
- Daniel Cheng
- Łukasz Anforowicz
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Diana Qu (Gerrit)
Diana Qu added 1 comment![]( "Open in Gerrit")
was_discarded_ = navigation_request->commit_params().was_discarded;Diana QuPlease fix this WARNING reported by ClangTidy: check: bugprone-use-after-move
Analyzer Description: Runs clang-tidy checks against chromium
Owner: gb...@chromium.orgcheck: bugprone-use-after-move
'navigation_request' used after it was moved (https://clang.llvm.org/extra/clang-tidy/checks/bugprone/use-after-move.html)
(Note: You can add `Skip-Clang-Tidy-Checks: bugprone-use-after-move` footer to the CL description to skip the check)
(Lint observed on `android-clang-tidy-rel` and `linux-clang-tidy-rel`)
I can just revert this line locally. It got added after running `git cl format`
Reverted all untouched change related to format run.
Related details
- Charlie Reis
- Daniel Cheng
- Łukasz Anforowicz
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Diana Qu (Gerrit)
Diana Qu added 1 comment![]( "Open in Gerrit")
Bumping up this CL a bit for some pending questions and discussions. Kindly review when you get a chance. Thank you
Related details
- Charlie Reis
- Daniel Cheng
- Łukasz Anforowicz
Code-Coverage
Code-Owners
Charlie Reis (Gerrit)
Charlie Reis added 6 comments![]( "Open in Gerrit")
Sorry for the delay-- two of these discussions needed a bit of investigation.
now passes, since MHTML was classified as same_doucument navigation,minor nit: document
// must remain exactly the same as the entry in session history.s/the entry in session history/before the commit/
(We aren't comparing to session history, but rather to the current state of the FrameTreeNode / RFH.)
GetProcess(), bad_message::RFH_INVALID_ORIGIN_ON_COMMIT);Diana QuAh, it's best not to reuse existing renderer kill codes, because that makes it harder to tell which one is occurring when we look at histograms. Let's use a new unique value here.
It's probably also worth referencing same-document in all of these codes, along the lines of:
RFH_SAME_DOC_ORIGIN_CHANGE
RFH_SAME_DOC_INSECURE_REQUEST_POLICY_CHANGE
RFH_SAME_DOC_INSECURE_NAV_SET_CHANGE
Diana QuUpdated.
I noticed that `CrossOriginSameDocumentCommit` and `CrossOriginSameDocumentCommitFromAboutBlank` fail after I change the error message code. Both this test expect `RFH_INVALID_ORIGIN_ON_COMMIT`, but since we do the origin check early now, it will return `RFH_SAME_DOC_ORIGIN_CHANGE`.
I prefer to just remove the origin check in this mismatch case since we are verifying origins in `CanCommitOriginAndUrl` later down the line and have a error message corresponding to it already. (Since it seems like we don't want to reuse these code?)
Hmm. It looks like the relevant existing check is at https://source.chromium.org/chromium/chromium/src/+/main:content/browser/renderer_host/render_frame_host_impl.cc;drc=e1bf12cc67dde1f974d2aa300fcfa340b140f5c5;l=11434-11440:
```
// Same-document navigations cannot change origins, as long as these checks
// aren't being bypassed in unusual modes. This check must be after the MHTML
// check, as shown by NavigationMhtmlBrowserTest.IframeAboutBlankNotFound.
if (is_same_document_navigation && origin != GetLastCommittedOrigin()) {
LogCanCommitOriginAndUrlFailureReason("cross_origin_same_document");
return CanCommitStatus::CANNOT_COMMIT_ORIGIN;
}
```
That does have a broad MHTML exemption [here](https://source.chromium.org/chromium/chromium/src/+/main:content/browser/renderer_host/render_frame_host_impl.cc;drc=e1bf12cc67dde1f974d2aa300fcfa340b140f5c5;l=11410-11419), and it even cites the test we're running into.
However, relying on that would mean we don't have a test that fails without the MHTML change, even though this CL would introduce a functional bug if we didn't have the MHTML change: MHTML about:blank#foo documents would have a new opaque origin in the renderer, but the browser process would ignore that origin on line 4931 because it claims to be same-document. That origin mismatch could lead to other types of renderer kills that are harder to diagnose, so I think we do want both test coverage and the MHTML change.
I agree with you that having overlapping renderer kills for this isn't ideal either, though. What if we removed the new RFH_SAME_DOC_ORIGIN_CHANGE and updated the existing check in RenderFrameHostImpl::CanCommitOriginAndUrl to catch the new bug? More specifically, we could move the `is_same_document_navigation` block above the `IsMhtmlSubframe` block, since we now need that particular invariant to apply to MHTML as well. The MHTML change should then prevent the `IframeAboutBlankNotFound` test from failing.
same_document_navigation = false;Diana QuI realized there's a problem with this approach, though I'm curious for dcheng@'s or lukasza@'s take on whether/how to fix it.
Here, we're unconditionally classifying all navigations within MHTML pages be cross-document, to handle the case where about:blank#foo in MHTML is actually cross-document (and thus changes origins).
Unfortunately, it is possible to have actual same-document navigations in MHTML, though apparently another bug makes that a little tricky. Imagine saving a page with a #foo link to MHTML (such as https://csreis.github.io/tests/same-document-navigations.html), loading the archive, and then clicking on the link. That should be same-document, but then this code would force it to be cross-document (and worse, it would result in an empty document because that #foo URL is not saved in the archive).
I've tried this in practice (including within a subframe) and there's a separate bug where the #foo link gets rewritten to include the full URL (https://csreis.github.io/tests/same-document-navigations.html#foo), and thus it doesn't look like a same-document URL from the MHTML's version of the URL. This means the #foo link is already basically broken today.
Using DevTools, I found it was technically still possible to navigate to "#foo" and get a same-document navigation, and I confirmed that this new code made it cross-document (and thus broken again). Without DevTools, I'm not sure if we can run into this problem, since MHTML can't run scripts (e.g., pushState), and all same-document links appear to be rewritten to leave the archive.
I'm not quite sure how we should approach this. Long term, it seems like we should fix the MHTML bug and allow same-document navigations, which would mean finding a different way to handle this case (e.g., only treating that first about:blank#foo navigation during archive loading as cross-document, but not all navigations within MHTML documents?). For now, it appears really hard to get a same-document navigation in MHTML to begin with (without opening DevTools), so maybe a TODO in this block isn't out of the question?
Łukasz AnforowiczDoesn't sounds like classify MTHML scope to only #framgent to cross-document is doable right now. I added some todos here just in case we decided to ship first and fix the #foo case later. (https://issues.chromium.org/issues/423663315). But I'm open with discussions.
I've tried this in practice (including within a subframe) and there's a separate bug where the #foo link gets rewritten to include the full URL (https://csreis.github.io/tests/same-document-navigations.html#foo), and thus it doesn't look like a same-document URL from the MHTML's version of the URL. This means the #foo link is already basically broken today.
What kind of broken-ness are we talking about?
- Classifying the navigation as same-document VS cross-document?
- Navigation going to the network, instead of staying within the MHTML archive?
If we're talking about the latter, then maybe we should (in a follow-up CL?) change how URLs are looked up in MHTML archive (discarding `#foo` from URLs before using them as a key). That said, I haven't looked at how the code actually does MHTML archive entry lookup, so maybe what I said here is inaccurate / misleading / wrong...
Sorry for the delay. It appears the type of brokenness depends on whether it's in the main frame or a subframe.
In a subframe, when a #foo link gets rewritten to https://csreis.github.io/tests/same-document-navigations.html#foo, clicking that link attempts to look it up in the archive and doesn't find anything, so we end up with a new empty document, instead of scrolling to the #foo anchor as it should.
In a main frame, links in MHTML documents are apparently allowed to go to the network, so the rewritten #foo link does a cross-document navigation to the real https://csreis.github.io/tests/same-document-navigations.html#foo URL.
Neither behavior is correct, since #foo should have done a scroll to the fragment in the same document in both cases.
And yes, I think we should try to fix MHTML's behavior in a separate CL, possibly by not rewriting fragment links.
The trouble is that this CL (as it stands) will prevent us from fixing that bug, because it will force all navigations in an MHTML document to be cross-document. Maybe we can get by with that today (with a risk of regressing any same-document navigations that actually do work today), but we'll need a more accurate classification to get same-document navigations to work while still calling the initial about:blank#fragment load a cross-document navigation.
name: "TranslationAPIV1",
status: {
"Win": "experimental",
"Mac": "experimental",
"Linux": "experimental",
"default": "",
},
copied_from_base_feature_if: "overridden",
},Is this due to a merge conflict? It seems unrelated to this CL.
Related details
- Daniel Cheng
- Diana Qu
- Łukasz Anforowicz
Diana Qu (Gerrit)
Diana Qu added 4 comments![]( "Open in Gerrit")
- As a result, NavigationMhtmlBrowserTest.IframeAboutBlankNotFound
now passes, since the navigation is no longer misclassified.Diana QuCan you help me when/how the test becomes broken? It's not broken today, so I assume that the test will break after step 1 (enforcing invariants on same-document navigations)?
Yes, it will break after step 1. because MHTML is classifying as same document navigation but in fact new document is committed and new opaque origins are created. (I can update the description with more details if that helps)
Done
now passes, since MHTML was classified as same_doucument navigation,Diana Quminor nit: document
Done
same_document_navigation = false;Diana QuI realized there's a problem with this approach, though I'm curious for dcheng@'s or lukasza@'s take on whether/how to fix it.
Here, we're unconditionally classifying all navigations within MHTML pages be cross-document, to handle the case where about:blank#foo in MHTML is actually cross-document (and thus changes origins).
Unfortunately, it is possible to have actual same-document navigations in MHTML, though apparently another bug makes that a little tricky. Imagine saving a page with a #foo link to MHTML (such as https://csreis.github.io/tests/same-document-navigations.html), loading the archive, and then clicking on the link. That should be same-document, but then this code would force it to be cross-document (and worse, it would result in an empty document because that #foo URL is not saved in the archive).
I've tried this in practice (including within a subframe) and there's a separate bug where the #foo link gets rewritten to include the full URL (https://csreis.github.io/tests/same-document-navigations.html#foo), and thus it doesn't look like a same-document URL from the MHTML's version of the URL. This means the #foo link is already basically broken today.
Using DevTools, I found it was technically still possible to navigate to "#foo" and get a same-document navigation, and I confirmed that this new code made it cross-document (and thus broken again). Without DevTools, I'm not sure if we can run into this problem, since MHTML can't run scripts (e.g., pushState), and all same-document links appear to be rewritten to leave the archive.
I'm not quite sure how we should approach this. Long term, it seems like we should fix the MHTML bug and allow same-document navigations, which would mean finding a different way to handle this case (e.g., only treating that first about:blank#foo navigation during archive loading as cross-document, but not all navigations within MHTML documents?). For now, it appears really hard to get a same-document navigation in MHTML to begin with (without opening DevTools), so maybe a TODO in this block isn't out of the question?
Łukasz AnforowiczDoesn't sounds like classify MTHML scope to only #framgent to cross-document is doable right now. I added some todos here just in case we decided to ship first and fix the #foo case later. (https://issues.chromium.org/issues/423663315). But I'm open with discussions.
Charlie ReisI've tried this in practice (including within a subframe) and there's a separate bug where the #foo link gets rewritten to include the full URL (https://csreis.github.io/tests/same-document-navigations.html#foo), and thus it doesn't look like a same-document URL from the MHTML's version of the URL. This means the #foo link is already basically broken today.
What kind of broken-ness are we talking about?
- Classifying the navigation as same-document VS cross-document?
- Navigation going to the network, instead of staying within the MHTML archive?
If we're talking about the latter, then maybe we should (in a follow-up CL?) change how URLs are looked up in MHTML archive (discarding `#foo` from URLs before using them as a key). That said, I haven't looked at how the code actually does MHTML archive entry lookup, so maybe what I said here is inaccurate / misleading / wrong...
Sorry for the delay. It appears the type of brokenness depends on whether it's in the main frame or a subframe.
In a subframe, when a #foo link gets rewritten to https://csreis.github.io/tests/same-document-navigations.html#foo, clicking that link attempts to look it up in the archive and doesn't find anything, so we end up with a new empty document, instead of scrolling to the #foo anchor as it should.
In a main frame, links in MHTML documents are apparently allowed to go to the network, so the rewritten #foo link does a cross-document navigation to the real https://csreis.github.io/tests/same-document-navigations.html#foo URL.
Neither behavior is correct, since #foo should have done a scroll to the fragment in the same document in both cases.
And yes, I think we should try to fix MHTML's behavior in a separate CL, possibly by not rewriting fragment links.
The trouble is that this CL (as it stands) will prevent us from fixing that bug, because it will force all navigations in an MHTML document to be cross-document. Maybe we can get by with that today (with a risk of regressing any same-document navigations that actually do work today), but we'll need a more accurate classification to get same-document navigations to work while still calling the initial about:blank#fragment load a cross-document navigation.
So the desired behavior is that we want to only reclassify the initial about:blank#fragment loads as cross doc and left other mhtml related navigation same doc still?
name: "TranslationAPIV1",
status: {
"Win": "experimental",
"Mac": "experimental",
"Linux": "experimental",
"default": "",
},
copied_from_base_feature_if: "overridden",
},Is this due to a merge conflict? It seems unrelated to this CL.
- Charlie Reis
- Daniel Cheng
- Łukasz Anforowicz
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Charlie Reis (Gerrit)
Charlie Reis added 1 comment![]( "Open in Gerrit")
That would be ideal, yes-- it would let us fix the existing MHTML bug so that same-document navigations would work, without this new code reclassifying them as cross-document.
At first glance, I'm not quite sure how to distinguish between that initial about:blank#fragment load (which may load from the archive and thus should be cross-document) and a later about:blank#fragment2 case which should be same-document. Maybe there's some way to distinguish them.
Related details
- Daniel Cheng
- Diana Qu
- Łukasz Anforowicz
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Diana Qu (Gerrit)
Diana Qu added 2 comments![]( "Open in Gerrit")
// must remain exactly the same as the entry in session history.s/the entry in session history/before the commit/
(We aren't comparing to session history, but rather to the current state of the FrameTreeNode / RFH.)
Updated. (Must be mixed up with the all the session history related invariant I was adding)
I remember seeing `has_committed_any_navigation()` and `is_initial_empty_document()` in renderer code. I assume there are equivalent code in frame document? And I also see `HasFragmentIdentifier` in url. Can we use that and combine with checking if the url is about:blank as a check and only set same_document_navigation = false in that case?
Related details
- Charlie Reis
- Daniel Cheng
- Łukasz Anforowicz
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Diana Qu (Gerrit)
Diana Qu added 1 comment![]( "Open in Gerrit")
GetProcess(), bad_message::RFH_INVALID_ORIGIN_ON_COMMIT);Diana QuAh, it's best not to reuse existing renderer kill codes, because that makes it harder to tell which one is occurring when we look at histograms. Let's use a new unique value here.
It's probably also worth referencing same-document in all of these codes, along the lines of:
RFH_SAME_DOC_ORIGIN_CHANGE
RFH_SAME_DOC_INSECURE_REQUEST_POLICY_CHANGE
RFH_SAME_DOC_INSECURE_NAV_SET_CHANGE
Diana QuUpdated.
Charlie ReisI noticed that `CrossOriginSameDocumentCommit` and `CrossOriginSameDocumentCommitFromAboutBlank` fail after I change the error message code. Both this test expect `RFH_INVALID_ORIGIN_ON_COMMIT`, but since we do the origin check early now, it will return `RFH_SAME_DOC_ORIGIN_CHANGE`.
I prefer to just remove the origin check in this mismatch case since we are verifying origins in `CanCommitOriginAndUrl` later down the line and have a error message corresponding to it already. (Since it seems like we don't want to reuse these code?)
Hmm. It looks like the relevant existing check is at https://source.chromium.org/chromium/chromium/src/+/main:content/browser/renderer_host/render_frame_host_impl.cc;drc=e1bf12cc67dde1f974d2aa300fcfa340b140f5c5;l=11434-11440:
```
// Same-document navigations cannot change origins, as long as these checks
// aren't being bypassed in unusual modes. This check must be after the MHTML
// check, as shown by NavigationMhtmlBrowserTest.IframeAboutBlankNotFound.
if (is_same_document_navigation && origin != GetLastCommittedOrigin()) {
LogCanCommitOriginAndUrlFailureReason("cross_origin_same_document");
return CanCommitStatus::CANNOT_COMMIT_ORIGIN;
}
```That does have a broad MHTML exemption [here](https://source.chromium.org/chromium/chromium/src/+/main:content/browser/renderer_host/render_frame_host_impl.cc;drc=e1bf12cc67dde1f974d2aa300fcfa340b140f5c5;l=11410-11419), and it even cites the test we're running into.
However, relying on that would mean we don't have a test that fails without the MHTML change, even though this CL would introduce a functional bug if we didn't have the MHTML change: MHTML about:blank#foo documents would have a new opaque origin in the renderer, but the browser process would ignore that origin on line 4931 because it claims to be same-document. That origin mismatch could lead to other types of renderer kills that are harder to diagnose, so I think we do want both test coverage and the MHTML change.
I agree with you that having overlapping renderer kills for this isn't ideal either, though. What if we removed the new RFH_SAME_DOC_ORIGIN_CHANGE and updated the existing check in RenderFrameHostImpl::CanCommitOriginAndUrl to catch the new bug? More specifically, we could move the `is_same_document_navigation` block above the `IsMhtmlSubframe` block, since we now need that particular invariant to apply to MHTML as well. The MHTML change should then prevent the `IframeAboutBlankNotFound` test from failing.
Updated.
Related details
- Charlie Reis
- Daniel Cheng
- Łukasz Anforowicz
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Diana Qu (Gerrit)
Diana Qu added 1 comment![]( "Open in Gerrit")
Something like the following? That way, `CANNOT_COMMIT_ORIGIN` check don't need to be moved.
```
if (parent->Loader().GetDocumentLoader()->HasBeenLoadedAsWebArchive()) {
const KURL& target = frame_->GetDocument()->Url();
if (target.ProtocolIs("about") && target.Host() == "blank" &&
target.HasFragmentIdentifier() &&
frame_->GetDocument()->IsInitialEmptyDocument()) {
same_document_navigation = false;
}
}
```
Diana Qu (Gerrit)
(If this works. I might move out the mhtml change to another CL for clarity)
Charlie Reis (Gerrit)
Charlie Reis added 8 comments![]( "Open in Gerrit")
Thanks! This might get us close enough that we can proceed without much concern.
2. Reclassify MHTML navigations as cross-documentWe can mention this is just in the initial empty document, based on the discussion below.
// aren't being bypassed in unusual modes. This check must be after the MHTML
// check, as shown by NavigationMhtmlBrowserTest.IframeAboutBlankNotFound.We can remove this sentence now.
if (is_same_document_navigation && origin != GetLastCommittedOrigin()) {We should guard this with the new feature and keep the old one for when the feature is disabled, just in case we miss an MHTML case that matters.
GetProcess(), bad_message::RFH_INVALID_ORIGIN_ON_COMMIT);Diana QuAh, it's best not to reuse existing renderer kill codes, because that makes it harder to tell which one is occurring when we look at histograms. Let's use a new unique value here.
It's probably also worth referencing same-document in all of these codes, along the lines of:
RFH_SAME_DOC_ORIGIN_CHANGE
RFH_SAME_DOC_INSECURE_REQUEST_POLICY_CHANGE
RFH_SAME_DOC_INSECURE_NAV_SET_CHANGE
Diana QuUpdated.
Charlie ReisI noticed that `CrossOriginSameDocumentCommit` and `CrossOriginSameDocumentCommitFromAboutBlank` fail after I change the error message code. Both this test expect `RFH_INVALID_ORIGIN_ON_COMMIT`, but since we do the origin check early now, it will return `RFH_SAME_DOC_ORIGIN_CHANGE`.
I prefer to just remove the origin check in this mismatch case since we are verifying origins in `CanCommitOriginAndUrl` later down the line and have a error message corresponding to it already. (Since it seems like we don't want to reuse these code?)
Diana QuHmm. It looks like the relevant existing check is at https://source.chromium.org/chromium/chromium/src/+/main:content/browser/renderer_host/render_frame_host_impl.cc;drc=e1bf12cc67dde1f974d2aa300fcfa340b140f5c5;l=11434-11440:
```
// Same-document navigations cannot change origins, as long as these checks
// aren't being bypassed in unusual modes. This check must be after the MHTML
// check, as shown by NavigationMhtmlBrowserTest.IframeAboutBlankNotFound.
if (is_same_document_navigation && origin != GetLastCommittedOrigin()) {
LogCanCommitOriginAndUrlFailureReason("cross_origin_same_document");
return CanCommitStatus::CANNOT_COMMIT_ORIGIN;
}
```That does have a broad MHTML exemption [here](https://source.chromium.org/chromium/chromium/src/+/main:content/browser/renderer_host/render_frame_host_impl.cc;drc=e1bf12cc67dde1f974d2aa300fcfa340b140f5c5;l=11410-11419), and it even cites the test we're running into.
However, relying on that would mean we don't have a test that fails without the MHTML change, even though this CL would introduce a functional bug if we didn't have the MHTML change: MHTML about:blank#foo documents would have a new opaque origin in the renderer, but the browser process would ignore that origin on line 4931 because it claims to be same-document. That origin mismatch could lead to other types of renderer kills that are harder to diagnose, so I think we do want both test coverage and the MHTML change.
I agree with you that having overlapping renderer kills for this isn't ideal either, though. What if we removed the new RFH_SAME_DOC_ORIGIN_CHANGE and updated the existing check in RenderFrameHostImpl::CanCommitOriginAndUrl to catch the new bug? More specifically, we could move the `is_same_document_navigation` block above the `IsMhtmlSubframe` block, since we now need that particular invariant to apply to MHTML as well. The MHTML change should then prevent the `IframeAboutBlankNotFound` test from failing.
Updated.
Thanks, looks good!
// This feature acts as a kill switch for https://crbug.com/40580002.Maybe also mention that this should not be enabled unless TreatMhtmlAsCrossDocument is also enabled in Blink, or else some MHTML navigations may cause renderer kills.
// Treat MHTML navigations as cross-document navigations, sinceAs noted above, let's limit this to cases within the initial empty document (and maybe rename the feature to TreatMhtmlInitialDocumentLoadsAsCrossDocument).
That's very close. I like the IsInitialEmptyDocument check, which might be the only time this reclassification to cross-document is needed. The checks for "about" and "blank" should be redundant with that, since I don't think any other protocol and host can be the initial empty document.
The only case that won't handle correctly is from my example above: about:blank#fragment should initially load as cross-document given how the MHTML code works, but then a later navigation to about:blank#fragment2 in theory should be allowed to be same-document, scrolling within the initial document.
Still, that case seems unlikely to matter in practice, and we would probably be in good shape if we just limited the re-classification to the IsInitialEmptyDocument cases. We can include a TODO just for that one corner case and not worry too much about it.
Related details
- Daniel Cheng
- Diana Qu
- Łukasz Anforowicz
Diana Qu (Gerrit)
Diana Qu added 1 comment![]( "Open in Gerrit")
I thought `IsInitialEmptyDocument or maybe `has commited any navigation` can distingush between `about::blank#fragment` and `about:blank#fragment2`? Since we already navitgated to `about::blank#fragment`?
Related details
- Charlie Reis
- Daniel Cheng
- Łukasz Anforowicz
Diana Qu (Gerrit)
Diana Qu added 1 comment![]( "Open in Gerrit")
// aren't being bypassed in unusual modes. This check must be after the MHTML
// check, as shown by NavigationMhtmlBrowserTest.IframeAboutBlankNotFound.We can remove this sentence now.
I believe if we do make the "only classify initial mhtml fragment cross document" change, this no long need to be moved before the mhtml check?
(Also do you mind me move the mhtml change to another CL and try land that first?)
Charlie Reis (Gerrit)
Charlie Reis added 2 comments![]( "Open in Gerrit")
// aren't being bypassed in unusual modes. This check must be after the MHTML
// check, as shown by NavigationMhtmlBrowserTest.IframeAboutBlankNotFound.Diana QuWe can remove this sentence now.
I believe if we do make the "only classify initial mhtml fragment cross document" change, this no long need to be moved before the mhtml check?
(Also do you mind me move the mhtml change to another CL and try land that first?)
I believe if we do make the "only classify initial mhtml fragment cross document" change, this no long need to be moved before the mhtml check?
I think we do want to move this before the MHTML check, to make the enforcement stronger. We're fixing the MHTML code so that the exception to the same-document enforcement isn't needed.
(Also do you mind me move the mhtml change to another CL and try land that first?)
That's certainly fine! Smaller CLs tend to be easier to reason about. It's possible to make this CL dependent on that one so that the tests still pass, since this one should require the MHTML fix.
Oh, is it no longer considered the initial empty document after doing the about:blank#fragment navigation? That's great, and it may solve this problem in general.
Related details
- Daniel Cheng
- Diana Qu
- Łukasz Anforowicz
Diana Qu (Gerrit)
Diana Qu added 2 comments![]( "Open in Gerrit")
// aren't being bypassed in unusual modes. This check must be after the MHTML
// check, as shown by NavigationMhtmlBrowserTest.IframeAboutBlankNotFound.Diana QuWe can remove this sentence now.
Charlie ReisI believe if we do make the "only classify initial mhtml fragment cross document" change, this no long need to be moved before the mhtml check?
(Also do you mind me move the mhtml change to another CL and try land that first?)
I believe if we do make the "only classify initial mhtml fragment cross document" change, this no long need to be moved before the mhtml check?
I think we do want to move this before the MHTML check, to make the enforcement stronger. We're fixing the MHTML code so that the exception to the same-document enforcement isn't needed.
(Also do you mind me move the mhtml change to another CL and try land that first?)
That's certainly fine! Smaller CLs tend to be easier to reason about. It's possible to make this CL dependent on that one so that the tests still pass, since this one should require the MHTML fix.
I somehow remember seeing `ForwardRedirectWithNoCommittedEntry` fail after making the initial classification change and move the check to early spot. I can double check again.
I think it make more sense to land the MHTML classification change first before we make this same document change.
Let me set a debugger and making sure. But that's how I understand the method. At least thats what it means for `is_initial_empty_document()` and `has_committed_any_navigation()` while I was looking into the renderer swap thing.
Related details
- Charlie Reis
- Daniel Cheng
- Łukasz Anforowicz
Diana Qu (Gerrit)
Diana Qu added 1 comment![]( "Open in Gerrit")
// aren't being bypassed in unusual modes. This check must be after the MHTML
// check, as shown by NavigationMhtmlBrowserTest.IframeAboutBlankNotFound.Diana QuWe can remove this sentence now.
Charlie ReisI believe if we do make the "only classify initial mhtml fragment cross document" change, this no long need to be moved before the mhtml check?
(Also do you mind me move the mhtml change to another CL and try land that first?)
Diana QuI believe if we do make the "only classify initial mhtml fragment cross document" change, this no long need to be moved before the mhtml check?
I think we do want to move this before the MHTML check, to make the enforcement stronger. We're fixing the MHTML code so that the exception to the same-document enforcement isn't needed.
(Also do you mind me move the mhtml change to another CL and try land that first?)
That's certainly fine! Smaller CLs tend to be easier to reason about. It's possible to make this CL dependent on that one so that the tests still pass, since this one should require the MHTML fix.
I somehow remember seeing `ForwardRedirectWithNoCommittedEntry` fail after making the initial classification change and move the check to early spot. I can double check again.
I think it make more sense to land the MHTML classification change first before we make this same document change.
Yah. `NavigationMhtmlBrowserTest.IframeAboutBlankNotFound` fails if we move the origin check above the mhtml check. `CANNOT_COMMIT_ORIGIN url 'about:blank#fragment' origin 'null [internally: (ABFA9BC760B77038FDF36AAABBF938F5) derived from file://]' lock '{ file:/// sandboxed }'`
I can look into it later.
Related details
- Charlie Reis
- Daniel Cheng
- Rakina Zata Amni
- Takashi Toyoshima
- Łukasz Anforowicz
Charlie Reis (Gerrit)
Charlie Reis added 2 comments![]( "Open in Gerrit")
Just adjusting the attention set to avoid having this in reviewer's queues, since it sounds like you're still looking into the test failure.
// aren't being bypassed in unusual modes. This check must be after the MHTML
// check, as shown by NavigationMhtmlBrowserTest.IframeAboutBlankNotFound.Diana QuWe can remove this sentence now.
Charlie ReisI believe if we do make the "only classify initial mhtml fragment cross document" change, this no long need to be moved before the mhtml check?
(Also do you mind me move the mhtml change to another CL and try land that first?)
Diana QuI believe if we do make the "only classify initial mhtml fragment cross document" change, this no long need to be moved before the mhtml check?
I think we do want to move this before the MHTML check, to make the enforcement stronger. We're fixing the MHTML code so that the exception to the same-document enforcement isn't needed.
(Also do you mind me move the mhtml change to another CL and try land that first?)
That's certainly fine! Smaller CLs tend to be easier to reason about. It's possible to make this CL dependent on that one so that the tests still pass, since this one should require the MHTML fix.
Diana QuI somehow remember seeing `ForwardRedirectWithNoCommittedEntry` fail after making the initial classification change and move the check to early spot. I can double check again.
I think it make more sense to land the MHTML classification change first before we make this same document change.
Yah. `NavigationMhtmlBrowserTest.IframeAboutBlankNotFound` fails if we move the origin check above the mhtml check. `CANNOT_COMMIT_ORIGIN url 'about:blank#fragment' origin 'null [internally: (ABFA9BC760B77038FDF36AAABBF938F5) derived from file://]' lock '{ file:/// sandboxed }'`
I can look into it later.
Interesting. I wonder if that means the about:blank#fragment case is still treated as same-document.
Related details
- Diana Qu
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Diana Qu (Gerrit)
Diana Qu added 1 comment![]( "Open in Gerrit")
// aren't being bypassed in unusual modes. This check must be after the MHTML
// check, as shown by NavigationMhtmlBrowserTest.IframeAboutBlankNotFound.Diana QuWe can remove this sentence now.
Charlie ReisI believe if we do make the "only classify initial mhtml fragment cross document" change, this no long need to be moved before the mhtml check?
(Also do you mind me move the mhtml change to another CL and try land that first?)
Diana QuI believe if we do make the "only classify initial mhtml fragment cross document" change, this no long need to be moved before the mhtml check?
I think we do want to move this before the MHTML check, to make the enforcement stronger. We're fixing the MHTML code so that the exception to the same-document enforcement isn't needed.
(Also do you mind me move the mhtml change to another CL and try land that first?)
That's certainly fine! Smaller CLs tend to be easier to reason about. It's possible to make this CL dependent on that one so that the tests still pass, since this one should require the MHTML fix.
Diana QuI somehow remember seeing `ForwardRedirectWithNoCommittedEntry` fail after making the initial classification change and move the check to early spot. I can double check again.
I think it make more sense to land the MHTML classification change first before we make this same document change.
Charlie ReisYah. `NavigationMhtmlBrowserTest.IframeAboutBlankNotFound` fails if we move the origin check above the mhtml check. `CANNOT_COMMIT_ORIGIN url 'about:blank#fragment' origin 'null [internally: (ABFA9BC760B77038FDF36AAABBF938F5) derived from file://]' lock '{ file:/// sandboxed }'`
I can look into it later.
Interesting. I wonder if that means the about:blank#fragment case is still treated as same-document.
I got it working. It was comparing the wrong value. Moving the mhtml change to another CL: https://chromium-review.googlesource.com/c/chromium/src/+/6653528. And I will clean up this one to be just same document restriction
Related details
- Charlie Reis