Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2615793002:
arc: Fix situation when ARC cannot be disabled safely. (Closed)
|
Created:
3 years, 11 months ago by khmel Modified:
3 years, 11 months ago CC:
chromium-reviews, elijahtaylor+arcwatch_chromium.org, yusukes+watch_chromium.org, hidehiko+watch_chromium.org, lhchavez+watch_chromium.org, oshima+watch_chromium.org, davemoore+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionarc: Fix situation when ARC cannot be disabled safely.
This add filtering error in case of ARC is disabled and try
to re-open UI app when request to show follows soon after
request to close.
BUG=b/34090891
TEST=Manually on device, cases described in bug no longer appear.
Review-Url: https://codereview.chromium.org/2615793002
Cr-Commit-Position: refs/heads/master@{#442485}
Committed: https://chromium.googlesource.com/chromium/src/+/a2407051a9a2c6e03125c8d959e1ef7d4e441e95
Patch Set 1 #
Total comments: 8
Patch Set 2 : add comment, discard arc_support_host fix #
Total comments: 8
Patch Set 3 : comments updated #Messages
Total messages: 13 (6 generated)
khmel@chromium.org changed reviewers: + hidehiko@chromium.org, xiyuan@chromium.org
Hi, PTAL
Thank you for finding an issue. Although the changes looks small enough, but the logic behind looks complicated. So, could you split this CL into two, ArcSessionManager's and ArcSupportHost's, because those are independent? Then, ArcSessionManager part can be landed smoothly, at least, I guess. https://codereview.chromium.org/2615793002/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_session_manager.cc (right): https://codereview.chromium.org/2615793002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_session_manager.cc:290: if (!IsArcEnabled()) { This check does not work in more complicated situation, unfortunately. E.g.; if "ARC is disabled, then reenabled, and then OnProvisioningFinished is called", this check passes through. Could you comment that this check is short-term workaround meanwhile, and what situation this is trying to address? https://codereview.chromium.org/2615793002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_session_manager.cc:291: LOG(WARNING) << " Provisioning result received after Arc was disabled. " Please remove a space between '"' and P. Ditto for '"' and I at the line below. Optional: How about raise the severity to ERROR, considering; 1) This is actually the case we should not hit (although, due to the current code structure, it is difficult to fix quickly). 2) This should be very rare, IMHO. ("Disabling ARC checkbox manually sounds not a common user action" multiplies "It needs to be done while provisioning and to happen the racy situation"). https://codereview.chromium.org/2615793002/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_support_host.cc (right): https://codereview.chromium.org/2615793002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_support_host.cc:372: // requested to show again soon. In this case OpenApplication may be ignored Who ignores the OpenApplication()? I couldn't find such handling code in, e.g., application_launch.cc etc.? Could you point me the condition? We used to hit a bug that multiple extension processes were launched crbug.com/651838. So, at least at the time, OpenApplication could launch multiple processes. Actually, this class's design relies on it. At the moment, I couldn't find design change. Do you know something? If there is, we need to re-design the whole mechanism. Or, if not, maybe there is some more specific condition where the window is not opened, so the specific one needs to be addressed, instead. WDYT? # Considering that, I skipped other comments about this block. I'll revisit here, when above things are resolved.
Thank you for taking a look. Please find updated version. https://codereview.chromium.org/2615793002/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_session_manager.cc (right): https://codereview.chromium.org/2615793002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_session_manager.cc:290: if (!IsArcEnabled()) { On 2017/01/05 08:18:02, hidehiko wrote: > This check does not work in more complicated situation, unfortunately. > E.g.; if "ARC is disabled, then reenabled, and then OnProvisioningFinished is > called", this check passes through. > > Could you comment that this check is short-term workaround meanwhile, > and what situation this is trying to address? Done. https://codereview.chromium.org/2615793002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_session_manager.cc:291: LOG(WARNING) << " Provisioning result received after Arc was disabled. " On 2017/01/05 08:18:02, hidehiko wrote: > Please remove a space between '"' and P. Ditto for '"' and I at the line below. > > Optional: How about raise the severity to ERROR, considering; > 1) This is actually the case we should not hit (although, due to the current > code structure, it is difficult to fix quickly). > 2) This should be very rare, IMHO. ("Disabling ARC checkbox manually sounds not > a common user action" multiplies "It needs to be done while provisioning and to > happen the racy situation"). The reason of this situation is the same as for line 302. So I would prefer WARNING here to be consistent. Imho this is not rare case. This happens always when ARC is disabled while requesting auth code and this is how mojo channel work. https://codereview.chromium.org/2615793002/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_support_host.cc (right): https://codereview.chromium.org/2615793002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_support_host.cc:372: // requested to show again soon. In this case OpenApplication may be ignored On 2017/01/05 08:18:02, hidehiko wrote: > Who ignores the OpenApplication()? > I couldn't find such handling code in, e.g., application_launch.cc etc.? > Could you point me the condition? > > We used to hit a bug that multiple extension processes were launched > crbug.com/651838. So, at least at the time, OpenApplication could launch > multiple processes. Actually, this class's design relies on it. At the moment, I > couldn't find design change. Do you know something? > If there is, we need to re-design the whole mechanism. > Or, if not, maybe there is some more specific condition where the window is not > opened, so the specific one needs to be addressed, instead. WDYT? > > # Considering that, I skipped other comments about this block. I'll revisit > here, when above things are resolved. Sorry, I don't exactly know code that prevents running multiple instanceso of this UI and currently have no time for such investigations. I have suspicion this due unique window id "play_store_wnd" in system. To be double sure I wrote test that run periodically RequestOpenApp. In my case I have only one window open and only one SetMessageHost regardless of how many times it was requested to open. Previous impl was built relying on this feature. If current design relies that multiple instances of platform app can be running then this might mean that design does reflect what actually happens in system and we might want revise it. Anyway, if you strictly against this fix I can drop this change because arc_session_manager fix is more important and with that fix this problem is much less visible.
hidehiko@chromium.org changed reviewers: + elijahtaylor@chromium.org
lgtm assuming comments will be revised. R+=elijahtaylor@, just in case for faster iteration, as I'm OOO on next Monday. https://codereview.chromium.org/2615793002/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_support_host.cc (right): https://codereview.chromium.org/2615793002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_support_host.cc:372: // requested to show again soon. In this case OpenApplication may be ignored On 2017/01/05 22:55:53, khmel wrote: > On 2017/01/05 08:18:02, hidehiko wrote: > > Who ignores the OpenApplication()? > > I couldn't find such handling code in, e.g., application_launch.cc etc.? > > Could you point me the condition? > > > > We used to hit a bug that multiple extension processes were launched > > crbug.com/651838. So, at least at the time, OpenApplication could launch > > multiple processes. Actually, this class's design relies on it. At the moment, > I > > couldn't find design change. Do you know something? > > If there is, we need to re-design the whole mechanism. > > Or, if not, maybe there is some more specific condition where the window is > not > > opened, so the specific one needs to be addressed, instead. WDYT? > > > > # Considering that, I skipped other comments about this block. I'll revisit > > here, when above things are resolved. > > Sorry, I don't exactly know code that prevents running multiple instanceso of > this UI and currently have no time for such investigations. I have suspicion > this due unique window id "play_store_wnd" in system. To be double sure I wrote > test that run periodically RequestOpenApp. In my case I have only one window > open and only one SetMessageHost regardless of how many times it was requested > to open. Previous impl was built relying on this feature. If current design > relies that multiple instances of platform app can be running then this might > mean that design does reflect what actually happens in system and we might want > revise it. > Anyway, if you strictly against this fix I can drop this change because > arc_session_manager fix is more important and with that fix this problem is much > less visible. I'm not against fixing ArcSupportHost, rather if there is a bug, we should fix it. However, in general, if the code author does not know the code and its background logic in details, we cannot say l-g-t-m. Specifically, this case has some contradiction with a past bug, so it looks more important. Note that, if "play_store_wnd" id is the root cause as you suspected (sorry I do not have enough time to investigate it today), it explains both the bug you're trying to fix and the past bug at a time. However, before moving forward, it is necessary to verify it rather than suspecting (Unfortunately, there are more layers, so running RequestOpenApp multiple times looks not support it enough...). Also, then, unfortunately, this code would introduces very complicated message passing flows which make maintenance very difficult, and actually would have a race issue, so it would be necessary to think about another way. Anyway, arc_session_manager only fix sounds more simpler, and easily understandable. Thank you for helping review. https://codereview.chromium.org/2615793002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_session_manager.cc (right): https://codereview.chromium.org/2615793002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager.cc:285: // Due the async nature of closing mojo channel, some sign in error may arrive "some sign in error" sounds ambiguous, because in future an engineer cannot figure out what error is expected and what is not. Maybe: If the Mojo message to notify finishing the provisioning is already sent from the container, it will be processed even after requesting to stop the container. Or something? https://codereview.chromium.org/2615793002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager.cc:286: // after stopping ARC. Ignore all results arriving after disabling ARC. In precise, "after disabling ARC" sounds misleading, because this does not support "disabling then reenabling" case. So, maybe: "Ignore all |result|s arriving while ARC is disabled, in order to avoid popping up an error message triggered below." should be less confusing. https://codereview.chromium.org/2615793002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager.cc:287: if (!IsArcEnabled()) { Please also explicitly comment that this code intentionally does not support the case of reenabling. Otherwise, if an engineer in future read this code, they would misunderstand this as an unintentional bug. https://codereview.chromium.org/2615793002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager.cc:289: << " Ignoring result " << static_cast<int>(result) << "."; Please address previous comment: > Ditto for '"' and I at the line below. , too.
Thank you for review. I added Xiyuan to reviewer to handle arc_support_host change. Once this change is out of the plate now so I ask Xiyuan to TBR (I suppose he simply missed this CL). https://codereview.chromium.org/2615793002/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_support_host.cc (right): https://codereview.chromium.org/2615793002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_support_host.cc:372: // requested to show again soon. In this case OpenApplication may be ignored On 2017/01/06 14:25:36, hidehiko wrote: > On 2017/01/05 22:55:53, khmel wrote: > > On 2017/01/05 08:18:02, hidehiko wrote: > > > Who ignores the OpenApplication()? > > > I couldn't find such handling code in, e.g., application_launch.cc etc.? > > > Could you point me the condition? > > > > > > We used to hit a bug that multiple extension processes were launched > > > crbug.com/651838. So, at least at the time, OpenApplication could launch > > > multiple processes. Actually, this class's design relies on it. At the > moment, > > I > > > couldn't find design change. Do you know something? > > > If there is, we need to re-design the whole mechanism. > > > Or, if not, maybe there is some more specific condition where the window is > > not > > > opened, so the specific one needs to be addressed, instead. WDYT? > > > > > > # Considering that, I skipped other comments about this block. I'll revisit > > > here, when above things are resolved. > > > > Sorry, I don't exactly know code that prevents running multiple instanceso of > > this UI and currently have no time for such investigations. I have suspicion > > this due unique window id "play_store_wnd" in system. To be double sure I > wrote > > test that run periodically RequestOpenApp. In my case I have only one window > > open and only one SetMessageHost regardless of how many times it was requested > > to open. Previous impl was built relying on this feature. If current design > > relies that multiple instances of platform app can be running then this might > > mean that design does reflect what actually happens in system and we might > want > > revise it. > > Anyway, if you strictly against this fix I can drop this change because > > arc_session_manager fix is more important and with that fix this problem is > much > > less visible. > > I'm not against fixing ArcSupportHost, rather if there is a bug, we should fix > it. > However, in general, if the code author does not know the code and its > background logic in details, we cannot say l-g-t-m. Specifically, this case has > some contradiction with a past bug, so it looks more important. > > Note that, if "play_store_wnd" id is the root cause as you suspected (sorry I do > not have enough time to investigate it today), it explains both the bug you're > trying to fix and the past bug at a time. However, before moving forward, it is > necessary to verify it rather than suspecting (Unfortunately, there are more > layers, so running RequestOpenApp multiple times looks not support it > enough...). Also, then, unfortunately, this code would introduces very > complicated message passing flows which make maintenance very difficult, and > actually would have a race issue, so it would be necessary to think about > another way. > > Anyway, arc_session_manager only fix sounds more simpler, and easily > understandable. Thank you for helping review. Acknowledged. https://codereview.chromium.org/2615793002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_session_manager.cc (right): https://codereview.chromium.org/2615793002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager.cc:285: // Due the async nature of closing mojo channel, some sign in error may arrive On 2017/01/06 14:25:36, hidehiko wrote: > "some sign in error" sounds ambiguous, because in future an engineer cannot > figure out what error is expected and what is not. > > Maybe: If the Mojo message to notify finishing the provisioning is already sent > from the container, it will be processed even after requesting to stop the > container. > > Or something? Done. https://codereview.chromium.org/2615793002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager.cc:286: // after stopping ARC. Ignore all results arriving after disabling ARC. On 2017/01/06 14:25:36, hidehiko wrote: > In precise, "after disabling ARC" sounds misleading, because this does not > support "disabling then reenabling" case. So, maybe: > > "Ignore all |result|s arriving while ARC is disabled, in order to avoid popping > up an error message triggered below." > > should be less confusing. Done. https://codereview.chromium.org/2615793002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager.cc:287: if (!IsArcEnabled()) { On 2017/01/06 14:25:36, hidehiko wrote: > Please also explicitly comment that this code intentionally does not support the > case of reenabling. > Otherwise, if an engineer in future read this code, they would misunderstand > this as an unintentional bug. Done. https://codereview.chromium.org/2615793002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager.cc:289: << " Ignoring result " << static_cast<int>(result) << "."; On 2017/01/06 14:25:36, hidehiko wrote: > Please address previous comment: > > > Ditto for '"' and I at the line below. > > , too. Done.
The CQ bit was checked by khmel@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from hidehiko@chromium.org Link to the patchset: https://codereview.chromium.org/2615793002/#ps40001 (title: "comments updated")
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": 1484014636318550,
"parent_rev": "e8604fbd50fd6e6b3e69f51fd79473df92276f96", "commit_rev":
"a2407051a9a2c6e03125c8d959e1ef7d4e441e95"}
Message was sent while issue was closed.
Description was changed from ========== arc: Fix situation when ARC cannot be disabled safely. This add filtering error in case of ARC is disabled and try to re-open UI app when request to show follows soon after request to close. BUG=b/34090891 TEST=Manually on device, cases described in bug no longer appear. ========== to ========== arc: Fix situation when ARC cannot be disabled safely. This add filtering error in case of ARC is disabled and try to re-open UI app when request to show follows soon after request to close. BUG=b/34090891 TEST=Manually on device, cases described in bug no longer appear. Review-Url: https://codereview.chromium.org/2615793002 Cr-Commit-Position: refs/heads/master@{#442485} Committed: https://chromium.googlesource.com/chromium/src/+/a2407051a9a2c6e03125c8d959e1... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/a2407051a9a2c6e03125c8d959e1... |
