Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2533473003:
Forbid content translate driver from initiating translation for page forward/back. (Closed)
|
Created:
4 years ago by leonhsl(Using Gerrit) Modified:
4 years ago CC:
chromium-reviews, jam, darin-cc_chromium.org, clamy Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionForbid content translate driver from initiating translation for page forward/back.
Except the real 'reload' of the same page, content translate driver
should not try to initiate translate manager explicitly by itself,
it should wait until page language determined notification from renderer.
BUG=653051
Committed: https://crrev.com/87468b3333378a63ada21fc67913839f72e7e9e9
Cr-Commit-Position: refs/heads/master@{#436577}
Patch Set 1 #Patch Set 2 : Block PAGE_TRANSITION_FORWARD_BACK specifically #Messages
Total messages: 36 (16 generated)
The CQ bit was checked by leon.han@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
leon.han@intel.com changed reviewers: + andrewhayden@chromium.org, droger@chromium.org, groby@chromium.org
PTAL, Thanks.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Looking at the CL and the bug, I don't think this is the right fix. It seems that the problem here is that the page is considered a reload whereas it is really not a reload. It seems strange that we could generate a transition that is both PAGE_TRANSITION_RELOAD and PAGE_TRANSITION_FORWARD_BACK. I feel that this is really the bug. Removing the transition qualifiers seems somewhat unrelated to this, and I that expect this CL as is would introduce new bugs. Things like tapping enter in the omnibox probably generate a transition that has the PAGE_TRANSITION_FROM_ADDRESS_BAR and would no longer be considered as a reload.
On 2016/11/25 09:56:08, droger wrote: > Looking at the CL and the bug, I don't think this is the right fix. > > It seems that the problem here is that the page is considered a reload whereas > it is really not a reload. > > It seems strange that we could generate a transition that is both > PAGE_TRANSITION_RELOAD and PAGE_TRANSITION_FORWARD_BACK. I feel that this is > really the bug. > Is it possible that PAGE_TRANSITION_RELOAD & PAGE_TRANSITION_FORWARD_BACK is valid transition and it was identified intentionally as not 'real' reload by content translate driver before? Then changing to compare the qualifiers causes mis-identifying it as a 'real' reload. > Removing the transition qualifiers seems somewhat unrelated to this, and I that > expect this CL as is would introduce new bugs. > Things like tapping enter in the omnibox probably generate a transition that has > the PAGE_TRANSITION_FROM_ADDRESS_BAR and would no longer be considered as a > reload. So does it mean that this omnibox bug had already been existing before https://chromium.googlesource.com/chromium/src/+/225012b72cdb52b2dd5a6184583d..., which actually sovled this omnibox bug? Maybe we should identify only the 2 transition values(PAGE_TRANSITION_RELOAD and PAGE_TRANSITION_FROM_ADDRESS_BAR) as 'real' reload for translate driver?
Description was changed from ========== Forbid content translate driver from initiating translation for page forward/back. Except the real 'reload' of the same page, content translate driver should not try to initiate translate manager explicitly by itself, it should wait until page language determined notification from renderer. BUG=653051 ========== to ========== Forbid content translate driver from initiating translation for page forward/back. Except the real 'reload' of the same page, content translate driver should not try to initiate translate manager explicitly by itself, it should wait until page language determined notification from renderer. BUG=653051 ==========
I discussed the issue with clamy@ who knows a lot about page transitions. According to her we should never get a page transition with both PAGE_TRANSITION_RELOAD and PAGE_TRANSITION_FORWARD_BACK, so my earlier comment is likely wrong. What is the transition type you observed when you did your fix? We should at least check that it was indeed not PAGE_TRANSITION_RELOAD & PAGE_TRANSITION_FORWARD_BACK (which would be a bug in the navigation code). In this specific scenario (reloading and going back to the NTP), NavigationEntryCommitted should be called twice: once for the reload and once for the back navigation for the NTP. And Translate should probably initiate a translation on the reload, but then cancel it when the back navigation happens. The bug is then that we probably either not receive the back navigation, or we receive it but fail to act on it. If this is the case, then the fix is not to change our behavior on the reload. We should rather understand why we do nothing on the back navigation. She also said that the NTP on android is a kind of specific case, and maybe trying to repro the issue on another page than the NTP might give insights into what happens.
Thanks a lot for explanations. Hope my description bellow would help us to move further:) Actually the original bug description is not correct, we can repro the issue easily, no need to do reload firstly before tapping back key, and NTP page is not necessary, too. The steps would be: 1, Open an English page. #Translate infobar does not show, this is correct. 2, Click a link or enter an url into address bar to open a Chinese page, wait until page loading completed totally. #Translate inforbar showes as expected. 3, Tap back key, we can observe that translate infobar disappeared, the previous English page begins showing, then the translate infobar showes again, which showing that the page language is Chinese. #Error! During the step No.3 above, I observed that NavigationEntryCommitted is called once, and it's definitely for the back navigation, because the target url is that English page. And I observed that at [1], load_details.type is content::NAVIGATION_TYPE_EXISTING_PAGE, and entry->GetTransitionType() is a big value unfortunately I did not identify it's exactly PAGE_TRANSITION_FORWARD_BACK or plus something else, but one thing is sure that [1] did not hit the true case and code continues running to [2], *HERE*, because translate_manager_->GetLanguageState() is still keeping old information of previous Chinese page, the wrong path begin! So, I think the back navigation should hit the true case at [1] then everything would be OK, then I checked content_translate_driver.cc change log to try to find some clue, then found https://chromium.googlesource.com/chromium/src/+/225012b72cdb52b2dd5a6184583d..., then I started to considering that it did break the intentional comparison before, then this CL born. [1] https://cs.chromium.org/chromium/src/components/translate/content/browser/con... [2] https://cs.chromium.org/chromium/src/components/translate/content/browser/con...
Confirmed that when tapping back key, NavigationEntryCommitted is called with the transition type 'PAGE_TRANSITION_FORWARD_BACK | PAGE_TRANSITION_RELOAD', and pull-to-fresh reload actually raises a 'PAGE_TRANSITION_RELOAD' type. We have 2 choices now: 1, Consider transition type 'PAGE_TRANSITION_FORWARD_BACK | PAGE_TRANSITION_RELOAD' as a BUG of navigation component, request them to solve. 2, Land this CL now to solve the original bug, personally I suppose it is regression caused by https://chromium.googlesource.com/chromium/src/+/225012b72cdb52b2dd5a6184583d....
clamy: do you know if this is something the navigation people should look into? On 2016/11/28 02:26:18, leonhsl wrote: > Confirmed that when tapping back key, NavigationEntryCommitted is called with > the transition type 'PAGE_TRANSITION_FORWARD_BACK | PAGE_TRANSITION_RELOAD', and > pull-to-fresh reload actually raises a 'PAGE_TRANSITION_RELOAD' type. > > We have 2 choices now: > 1, Consider transition type 'PAGE_TRANSITION_FORWARD_BACK | > PAGE_TRANSITION_RELOAD' as a BUG of navigation component, request them to solve. > 2, Land this CL now to solve the original bug, personally I suppose it is > regression caused by > https://chromium.googlesource.com/chromium/src/+/225012b72cdb52b2dd5a6184583d.... I'm inclined toward 1. I think the CL you linked was doing the right thing, and it just happened to surface another bug. Reverting it would just be a workaround, and will introduce more bugs. If the bug is high priority to fix, I can suggest a different, more targetted workaround: if (entry->GetTransitionType() & ui::PAGE_TRANSITION_FORWARD_BACK) { // Workaround for http://crbug.com/653051: back navigation sometimes have // the reload core type. return; }
clamy@chromium.org changed reviewers: + clamy@chromium.org
Yes that seems to be bug to me. I've filed crbug.com/669008 to track it. I would prefer if we fixed the underlying transition issue, since other components may not be expecting it.
On 2016/11/28 11:01:48, clamy (slow) wrote: > Yes that seems to be bug to me. I've filed crbug.com/669008 to track it. I would > prefer if we fixed the underlying transition issue, since other components may > not be expecting it. I'm all for fixing the root cause, but that's also an active bug for translations, so maybe we can land this as an interim fix? leonhsl@: Any chance to add a test for this so we can prevent regressions?
On 2016/11/30 21:33:43, groby wrote: > On 2016/11/28 11:01:48, clamy (slow) wrote: > > Yes that seems to be bug to me. I've filed crbug.com/669008 to track it. I > would > > prefer if we fixed the underlying transition issue, since other components may > > not be expecting it. > > I'm all for fixing the root cause, but that's also an active bug for > translations, so maybe we can land this as an interim fix? > > leonhsl@: Any chance to add a test for this so we can prevent regressions? I would be ok to land a workaround but not the patch set 1, which I think is too heavy handed and will introduce other bugs. We could specifically target these transitions that are both reload and back at the same time. See my suggestion in my earlier comment for a code snippet.
On 2016/12/01 00:16:48, droger wrote: > On 2016/11/30 21:33:43, groby wrote: > > On 2016/11/28 11:01:48, clamy (slow) wrote: > > > Yes that seems to be bug to me. I've filed crbug.com/669008 to track it. I > > would > > > prefer if we fixed the underlying transition issue, since other components > may > > > not be expecting it. > > > > I'm all for fixing the root cause, but that's also an active bug for > > translations, so maybe we can land this as an interim fix? > > > > leonhsl@: Any chance to add a test for this so we can prevent regressions? > > I would be ok to land a workaround but not the patch set 1, which I think is too > heavy handed and will introduce other bugs. We could specifically target these > transitions that are both reload and back at the same time. See my suggestion in > my earlier comment for a code snippet. Hi droger@, I'm still confusing why https://chromium.googlesource.com/chromium/src/+/225012b72cdb52b2dd5a6184583d... is supposed doing the right thing, with the fact that it raised such a regression. And I think this CL is somehow reverting it for translate to go back to original logic(targeting on PAGE_TRANSITION_RELOAD precisely&intentionally) which I think is correct because the original logic had been existing for so long time without any related bug reported,, would you please let me know what possible bug will it introduce now? I can help to confirm. Hi groby@, until now I have no clear idea how to add a test for this..
On 2016/12/02 02:55:22, leonhsl wrote: > On 2016/12/01 00:16:48, droger wrote: > > On 2016/11/30 21:33:43, groby wrote: > > > On 2016/11/28 11:01:48, clamy (slow) wrote: > > > > Yes that seems to be bug to me. I've filed crbug.com/669008 to track it. I > > > would > > > > prefer if we fixed the underlying transition issue, since other components > > may > > > > not be expecting it. > > > > > > I'm all for fixing the root cause, but that's also an active bug for > > > translations, so maybe we can land this as an interim fix? > > > > > > leonhsl@: Any chance to add a test for this so we can prevent regressions? > > > > I would be ok to land a workaround but not the patch set 1, which I think is > too > > heavy handed and will introduce other bugs. We could specifically target these > > transitions that are both reload and back at the same time. See my suggestion > in > > my earlier comment for a code snippet. > > Hi droger@, I'm still confusing why > https://chromium.googlesource.com/chromium/src/+/225012b72cdb52b2dd5a6184583d... > is supposed doing the right thing, with the fact that it raised such a > regression. And I think this CL is somehow reverting it for translate to go back > to original logic(targeting on PAGE_TRANSITION_RELOAD precisely&intentionally) > which I think is correct because the original logic had been existing for so > long time without any related bug reported,, would you please let me know what > possible bug will it introduce now? I can help to confirm. > > Hi groby@, until now I have no clear idea how to add a test for this.. A page transition is composed of a "core type" and a set of "qualifiers". See https://cs.chromium.org/chromium/src/ui/base/page_transition_types.h?rcl=0&l=108 for the list of flags. These two things are largely independent, and I don't know why they were merged into a single int (probably for performance reasons?). The consequence is that it is never OK to work with PageTransitions directly. They must always be decomposed into a core type and qualifiers. In translate here, we want to detect reloads, so only the core type is relevant. It does not matter if there was a redirect for example. Patch set 1 enforces that there are no qualifiers for the translation to happen. Thus I expect that it would break translate when reloading a page that has a server redirect or a client redirect (or any other qualifier, but I don't really know what triggers them). If there are some qualifiers we want to block, we should block them individually, on a case by case basis, rather than block everything. I would be fine for example rejecting reloads that have PAGE_TRANSITION_FORWARD_BACK or PAGE_TRANSITION_BLOCKED. But I think rejecting redirects would be a bug for example. According to clamy@, PAGE_TRANSITION_FORWARD_BACK should not never happen, so in theory we should not even need to block it. However, it does no harm to block it, so that would be a pretty good workaround in my opinion.
Patchset #2 (id:20001) has been deleted
On 2016/12/02 11:07:41, droger wrote: > On 2016/12/02 02:55:22, leonhsl wrote: > > On 2016/12/01 00:16:48, droger wrote: > > > On 2016/11/30 21:33:43, groby wrote: > > > > On 2016/11/28 11:01:48, clamy (slow) wrote: > > > > > Yes that seems to be bug to me. I've filed crbug.com/669008 to track it. > I > > > > would > > > > > prefer if we fixed the underlying transition issue, since other > components > > > may > > > > > not be expecting it. > > > > > > > > I'm all for fixing the root cause, but that's also an active bug for > > > > translations, so maybe we can land this as an interim fix? > > > > > > > > leonhsl@: Any chance to add a test for this so we can prevent regressions? > > > > > > I would be ok to land a workaround but not the patch set 1, which I think is > > too > > > heavy handed and will introduce other bugs. We could specifically target > these > > > transitions that are both reload and back at the same time. See my > suggestion > > in > > > my earlier comment for a code snippet. > > > > Hi droger@, I'm still confusing why > > > https://chromium.googlesource.com/chromium/src/+/225012b72cdb52b2dd5a6184583d... > > is supposed doing the right thing, with the fact that it raised such a > > regression. And I think this CL is somehow reverting it for translate to go > back > > to original logic(targeting on PAGE_TRANSITION_RELOAD precisely&intentionally) > > which I think is correct because the original logic had been existing for so > > long time without any related bug reported,, would you please let me know what > > possible bug will it introduce now? I can help to confirm. > > > > Hi groby@, until now I have no clear idea how to add a test for this.. > > A page transition is composed of a "core type" and a set of "qualifiers". > See > https://cs.chromium.org/chromium/src/ui/base/page_transition_types.h?rcl=0&l=108 > for the list of flags. > > These two things are largely independent, and I don't know why they were merged > into a single int (probably for performance reasons?). > The consequence is that it is never OK to work with PageTransitions directly. > They must always be decomposed into a core type and qualifiers. > > In translate here, we want to detect reloads, so only the core type is relevant. > It does not matter if there was a redirect for example. > > Patch set 1 enforces that there are no qualifiers for the translation to happen. > Thus I expect that it would break translate when reloading a page that has a > server redirect or a client redirect (or any other qualifier, but I don't really > know what triggers them). > > > If there are some qualifiers we want to block, we should block them > individually, on a case by case basis, rather than block everything. > I would be fine for example rejecting reloads that have > PAGE_TRANSITION_FORWARD_BACK or PAGE_TRANSITION_BLOCKED. > But I think rejecting redirects would be a bug for example. > > According to clamy@, PAGE_TRANSITION_FORWARD_BACK should not never happen, so in > theory we should not even need to block it. However, it does no harm to block > it, so that would be a pretty good workaround in my opinion. According from comments in page_transition_types.h and discussions in crbug.com/669008, 'PAGE_TRANSITION_RELOAD | PAGE_TRANSITION_FORWARD_BACK' is a valid transition type by design, although it's easy to be misunderstood. I would rather trust that the original translate logic is also by design, only PAGE_TRANSITION_RELOAD without any qualifier should be considered as a real reload inside ContentTranslateDriver::NavigationEntryCommitted(), this is intentional, and in fact it did not raise any bug for long time. Although now I can't find out specifically who's written these codes to help confirm directly... Hi, groby@, WDYT? Thanks.
On 2016/12/04 14:48:54, leonhsl wrote: > According from comments in page_transition_types.h and discussions in > crbug.com/669008, 'PAGE_TRANSITION_RELOAD | PAGE_TRANSITION_FORWARD_BACK' is a > valid transition type by design, although it's easy to be misunderstood. I would > rather trust that the original translate logic is also by design, only > PAGE_TRANSITION_RELOAD without any qualifier should be considered as a real > reload inside ContentTranslateDriver::NavigationEntryCommitted(), this is > intentional, and in fact it did not raise any bug for long time. Although now I > can't find out specifically who's written these codes to help confirm > directly... > > Hi, groby@, WDYT? Thanks. The bare minimum would be to use PageTransitionTypeIncludingQualifiersIs() instead of ==. The code would at least make some sense. As pointed in the bug though, there are legitimate cases where both reload and back/forward are defined together, i.e.: - visit page A, reload the page A, and navigate to page B, then history back to the page A and I think your suggestion would break translate in these cases.
On 2016/12/04 14:48:54, leonhsl wrote: > On 2016/12/02 11:07:41, droger wrote: > > On 2016/12/02 02:55:22, leonhsl wrote: > > > On 2016/12/01 00:16:48, droger wrote: > > > > On 2016/11/30 21:33:43, groby wrote: > > > > > On 2016/11/28 11:01:48, clamy (slow) wrote: > > > > > > Yes that seems to be bug to me. I've filed crbug.com/669008 to track > it. > > I > > > > > would > > > > > > prefer if we fixed the underlying transition issue, since other > > components > > > > may > > > > > > not be expecting it. > > > > > > > > > > I'm all for fixing the root cause, but that's also an active bug for > > > > > translations, so maybe we can land this as an interim fix? > > > > > > > > > > leonhsl@: Any chance to add a test for this so we can prevent > regressions? > > > > > > > > I would be ok to land a workaround but not the patch set 1, which I think > is > > > too > > > > heavy handed and will introduce other bugs. We could specifically target > > these > > > > transitions that are both reload and back at the same time. See my > > suggestion > > > in > > > > my earlier comment for a code snippet. > > > > > > Hi droger@, I'm still confusing why > > > > > > https://chromium.googlesource.com/chromium/src/+/225012b72cdb52b2dd5a6184583d... > > > is supposed doing the right thing, with the fact that it raised such a > > > regression. And I think this CL is somehow reverting it for translate to go > > back > > > to original logic(targeting on PAGE_TRANSITION_RELOAD > precisely&intentionally) > > > which I think is correct because the original logic had been existing for so > > > long time without any related bug reported,, would you please let me know > what > > > possible bug will it introduce now? I can help to confirm. > > > > > > Hi groby@, until now I have no clear idea how to add a test for this.. > > > > A page transition is composed of a "core type" and a set of "qualifiers". > > See > > > https://cs.chromium.org/chromium/src/ui/base/page_transition_types.h?rcl=0&l=108 > > for the list of flags. > > > > These two things are largely independent, and I don't know why they were > merged > > into a single int (probably for performance reasons?). > > The consequence is that it is never OK to work with PageTransitions directly. > > They must always be decomposed into a core type and qualifiers. > > > > In translate here, we want to detect reloads, so only the core type is > relevant. > > It does not matter if there was a redirect for example. > > > > Patch set 1 enforces that there are no qualifiers for the translation to > happen. > > Thus I expect that it would break translate when reloading a page that has a > > server redirect or a client redirect (or any other qualifier, but I don't > really > > know what triggers them). > > > > > > If there are some qualifiers we want to block, we should block them > > individually, on a case by case basis, rather than block everything. > > I would be fine for example rejecting reloads that have > > PAGE_TRANSITION_FORWARD_BACK or PAGE_TRANSITION_BLOCKED. > > But I think rejecting redirects would be a bug for example. > > > > According to clamy@, PAGE_TRANSITION_FORWARD_BACK should not never happen, so > in > > theory we should not even need to block it. However, it does no harm to block > > it, so that would be a pretty good workaround in my opinion. > > According from comments in page_transition_types.h and discussions in > crbug.com/669008, 'PAGE_TRANSITION_RELOAD | PAGE_TRANSITION_FORWARD_BACK' is a > valid transition type by design, although it's easy to be misunderstood. I would > rather trust that the original translate logic is also by design, only > PAGE_TRANSITION_RELOAD without any qualifier should be considered as a real > reload inside ContentTranslateDriver::NavigationEntryCommitted(), this is > intentional, and in fact it did not raise any bug for long time. Although now I > can't find out specifically who's written these codes to help confirm > directly... > > Hi, groby@, WDYT? Thanks. I think droger is the better contact to sort out questions around page transitions - I'm only vaguely familiar with them. > rather trust that the original translate logic is also by design Unless you verify this by looking up the origins, examining the comments on that CL, and verifying that at that point we already had core and qualifiers in the same int, that's a dangerous assumption. Chrome changes things fairly often, and given that this does not have any kind of testing associated, it is easily possible that this is a bug instead of a conscious decision. I wish I had better news :( (And that's why I asked for a test - so other people don't have to debate the same question again in 2 or 3 years)
The CQ bit was checked by leon.han@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks droger@ and groby@ a lot for all the discussions, based on your comments I agree the most appropriate&secure solution now for the bug is to block PAGE_TRANSITION_FORWARD_BACK specifically just as droger@'s suggestion. Uploaded ps#2, PTAnL, Thanks.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm thanks!
The CQ bit was checked by leon.han@intel.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1481026647652310,
"parent_rev": "5b57004feabe48ae95036d9755b5f9ae856b6ce7", "commit_rev":
"daad2977a1b5baa60d0cb9325c6ceb9e54834ec3"}
Message was sent while issue was closed.
Description was changed from ========== Forbid content translate driver from initiating translation for page forward/back. Except the real 'reload' of the same page, content translate driver should not try to initiate translate manager explicitly by itself, it should wait until page language determined notification from renderer. BUG=653051 ========== to ========== Forbid content translate driver from initiating translation for page forward/back. Except the real 'reload' of the same page, content translate driver should not try to initiate translate manager explicitly by itself, it should wait until page language determined notification from renderer. BUG=653051 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Forbid content translate driver from initiating translation for page forward/back. Except the real 'reload' of the same page, content translate driver should not try to initiate translate manager explicitly by itself, it should wait until page language determined notification from renderer. BUG=653051 ========== to ========== Forbid content translate driver from initiating translation for page forward/back. Except the real 'reload' of the same page, content translate driver should not try to initiate translate manager explicitly by itself, it should wait until page language determined notification from renderer. BUG=653051 Committed: https://crrev.com/87468b3333378a63ada21fc67913839f72e7e9e9 Cr-Commit-Position: refs/heads/master@{#436577} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/87468b3333378a63ada21fc67913839f72e7e9e9 Cr-Commit-Position: refs/heads/master@{#436577} |
