Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12223106:
Properly Remove Autofill Keyboard Listener. (Closed)
|
Created:
7 years, 10 months ago by csharp Modified:
7 years, 10 months ago CC:
chromium-reviews, Raman Kakilate, jam, benquan, dhollowa+watch_chromium.org, ahutter, browser-components-watch_chromium.org, dbeam+watch-autofill_chromium.org, joi+watch-content_chromium.org, darin-cc_chromium.org, Dane Wallinga, dyu1, estade+watch_chromium.org, Albert Bodenhamer Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionProperly Remove Autofill Keyboard Listener.
Remove the keyboard listener before we implicitly delete the object. Also
update the removal code to catch problems like this if they occur again.
R=estade@chromium.org
BUG=175454
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=182751
Patch Set 1 #
Total comments: 5
Patch Set 2 : #
Total comments: 3
Patch Set 3 : Remove inform_delegate_of_of_destruction #
Total comments: 4
Patch Set 4 : Merge Hide and HideInternal #Patch Set 5 : Fixing broken tests #
Total comments: 5
Patch Set 6 : #Patch Set 7 : #
Total comments: 4
Patch Set 8 : #Messages
Total messages: 27 (0 generated)
isherman@ for autofill_external_delegate.cc ben@ for render_widiget_host_impl.cc
I'm surprised there aren't any tests that cover this (or did they just fail to crash?)
We didn't have any tests that tests this behavior. I'm not sure how easy it would be to write a browser test to do this. It would need to: 1) Get the popup to appear for an field (to add the listener). 2) Get another popup to appear (different field or same field after making it disappear first) 3) Add key presses that will cause the listener to hit to deleted element.
Can we instead get rid of inform_delegate_of_destruction_ from autofill_popup_controller_impl.cc? It doesn't seem like we really need a separate codepath for the AutofillExternalDelegate anymore.
https://codereview.chromium.org/12223106/diff/1/content/browser/renderer_host... File content/browser/renderer_host/render_widget_host_impl.cc (right): https://codereview.chromium.org/12223106/diff/1/content/browser/renderer_host... content/browser/renderer_host/render_widget_host_impl.cc:1217: // Ensure that the element in actually in the list. nit: first "in" -> "is" https://codereview.chromium.org/12223106/diff/1/content/browser/renderer_host... content/browser/renderer_host/render_widget_host_impl.cc:1219: listener) != keyboard_listeners_.end()); I don't follow how this helps us spot this class of bug. This would allow us to detect double-removes, but that's not what's happening in this bug, right?
I'm not sure what you mean by removing inform_delegate_of_destruction_ https://chromiumcodereview.appspot.com/12223106/diff/1/content/browser/render... File content/browser/renderer_host/render_widget_host_impl.cc (right): https://chromiumcodereview.appspot.com/12223106/diff/1/content/browser/render... content/browser/renderer_host/render_widget_host_impl.cc:1217: // Ensure that the element in actually in the list. On 2013/02/12 20:57:57, Ilya Sherman wrote: > nit: first "in" -> "is" Done. https://chromiumcodereview.appspot.com/12223106/diff/1/content/browser/render... content/browser/renderer_host/render_widget_host_impl.cc:1219: listener) != keyboard_listeners_.end()); On 2013/02/12 20:57:57, Ilya Sherman wrote: > I don't follow how this helps us spot this class of bug. This would allow us to > detect double-removes, but that's not what's happening in this bug, right? Yup, this bug here is that the Autofill controller was deleted before we got here, so the pointer that is passed in is NULL (which doesn't appear in the list so we assert).
https://chromiumcodereview.appspot.com/12223106/diff/1/content/browser/render... File content/browser/renderer_host/render_widget_host_impl.cc (right): https://chromiumcodereview.appspot.com/12223106/diff/1/content/browser/render... content/browser/renderer_host/render_widget_host_impl.cc:1219: listener) != keyboard_listeners_.end()); On 2013/02/12 21:02:37, csharp wrote: > On 2013/02/12 20:57:57, Ilya Sherman wrote: > > I don't follow how this helps us spot this class of bug. This would allow us > to > > detect double-removes, but that's not what's happening in this bug, right? > > Yup, this bug here is that the Autofill controller was deleted before we got > here, so the pointer that is passed in is NULL (which doesn't appear in the list > so we assert). Ah, I thought we were passing in a pointer to memory that we no longer owned; but if the pointer gets NULLed out, this makes sense. Thanks.
Removed the inform_delegate_of_destruction_ code.
https://chromiumcodereview.appspot.com/12223106/diff/6001/chrome/browser/auto... File chrome/browser/autofill/autofill_external_delegate.cc (right): https://chromiumcodereview.appspot.com/12223106/diff/6001/chrome/browser/auto... chrome/browser/autofill/autofill_external_delegate.cc:67: controller_->Hide(); Should we just call HideAutofillPopup() here to make sure we properly deregister the listener?
LGTM with nits: https://chromiumcodereview.appspot.com/12223106/diff/6001/chrome/browser/auto... File chrome/browser/autofill/autofill_external_delegate.cc (right): https://chromiumcodereview.appspot.com/12223106/diff/6001/chrome/browser/auto... chrome/browser/autofill/autofill_external_delegate.cc:67: controller_->Hide(); On 2013/02/12 21:13:36, dtrainor wrote: > Should we just call HideAutofillPopup() here to make sure we properly deregister > the listener? Yeah, probably a good idea. https://chromiumcodereview.appspot.com/12223106/diff/9001/chrome/browser/auto... File chrome/browser/autofill/autofill_external_delegate.cc (right): https://chromiumcodereview.appspot.com/12223106/diff/9001/chrome/browser/auto... chrome/browser/autofill/autofill_external_delegate.cc:283: } nit: No need for curlies. https://chromiumcodereview.appspot.com/12223106/diff/9001/chrome/browser/ui/a... File chrome/browser/ui/autofill/autofill_popup_controller_impl.cc (right): https://chromiumcodereview.appspot.com/12223106/diff/9001/chrome/browser/ui/a... chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:173: HideInternal(); nit: No longer a need for HideInternal() to be distinct from Hide(); let's collapse those two methods down into just one.
https://chromiumcodereview.appspot.com/12223106/diff/6001/chrome/browser/auto... File chrome/browser/autofill/autofill_external_delegate.cc (right): https://chromiumcodereview.appspot.com/12223106/diff/6001/chrome/browser/auto... chrome/browser/autofill/autofill_external_delegate.cc:67: controller_->Hide(); On 2013/02/12 21:13:36, dtrainor wrote: > Should we just call HideAutofillPopup() here to make sure we properly deregister > the listener? No longer needed, Hide will properly deregister the listener
https://chromiumcodereview.appspot.com/12223106/diff/9001/chrome/browser/auto... File chrome/browser/autofill/autofill_external_delegate.cc (right): https://chromiumcodereview.appspot.com/12223106/diff/9001/chrome/browser/auto... chrome/browser/autofill/autofill_external_delegate.cc:283: } On 2013/02/12 21:15:25, Ilya Sherman wrote: > nit: No need for curlies. Done. https://chromiumcodereview.appspot.com/12223106/diff/9001/chrome/browser/ui/a... File chrome/browser/ui/autofill/autofill_popup_controller_impl.cc (right): https://chromiumcodereview.appspot.com/12223106/diff/9001/chrome/browser/ui/a... chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:173: HideInternal(); On 2013/02/12 21:15:25, Ilya Sherman wrote: > nit: No longer a need for HideInternal() to be distinct from Hide(); let's > collapse those two methods down into just one. Done.
lgtm
I had to made a few changes to fix some tests that were broken by the added DCHECK. PTAL
https://codereview.chromium.org/12223106/diff/5007/chrome/browser/autofill/au... File chrome/browser/autofill/autofill_external_delegate.h (right): https://codereview.chromium.org/12223106/diff/5007/chrome/browser/autofill/au... chrome/browser/autofill/autofill_external_delegate.h:191: content::RenderViewHost* registered_keyboard_listener_with_; If this is only needed for tests, then you should make the change in test code, not in the production code.
https://codereview.chromium.org/12223106/diff/5007/chrome/browser/autofill/au... File chrome/browser/autofill/autofill_external_delegate.h (right): https://codereview.chromium.org/12223106/diff/5007/chrome/browser/autofill/au... chrome/browser/autofill/autofill_external_delegate.h:191: content::RenderViewHost* registered_keyboard_listener_with_; On 2013/02/13 21:58:12, Ilya Sherman wrote: > If this is only needed for tests, then you should make the change in test code, > not in the production code. I'm not if this is only needed for tests. It was one of the browser tests that triggered the problem (the RenderViewHost at construction was different from the one when the popup was hidden), so I think that it could occur in the wild (although I haven't repo it. So I think this should stay, since it might occur in production.
https://codereview.chromium.org/12223106/diff/5007/chrome/browser/autofill/au... File chrome/browser/autofill/autofill_external_delegate.h (right): https://codereview.chromium.org/12223106/diff/5007/chrome/browser/autofill/au... chrome/browser/autofill/autofill_external_delegate.h:191: content::RenderViewHost* registered_keyboard_listener_with_; On 2013/02/13 22:16:04, csharp wrote: > On 2013/02/13 21:58:12, Ilya Sherman wrote: > > If this is only needed for tests, then you should make the change in test > code, > > not in the production code. > > I'm not if this is only needed for tests. It was one of the browser tests that > triggered the problem (the RenderViewHost at construction was different from the > one when the popup was hidden), so I think that it could occur in the wild > (although I haven't repo it. > > So I think this should stay, since it might occur in production. As far as I understand, it really shouldn't be possible for the RenderViewHost to change between construction and when the popup is hidden. The RenderViewHost should not change when there is no page navigation, and the popup should be dismissed prior to page navigation. Let's do some more investigation to understand why the test is failing.
Fixed the test, and went back to the old system. I was doing too many navigates and I think that was causing trouble because the autofill_external_delegate was then living longer than it should have. And ping ben@ for owner's approval.
lgtm
Charlie, could you take a look and give your advice on how best to approach safely unregistering ourselves as a listener from the correct RenderViewHost? Is there perhaps a way to guarantee that we'll be notified that a navigation is (definitely) about to happen *prior* to the old RenderViewHost being swapped out? https://chromiumcodereview.appspot.com/12223106/diff/5007/chrome/browser/auto... File chrome/browser/autofill/autofill_external_delegate.h (right): https://chromiumcodereview.appspot.com/12223106/diff/5007/chrome/browser/auto... chrome/browser/autofill/autofill_external_delegate.h:191: content::RenderViewHost* registered_keyboard_listener_with_; On 2013/02/14 06:19:18, Ilya Sherman wrote: > On 2013/02/13 22:16:04, csharp wrote: > > On 2013/02/13 21:58:12, Ilya Sherman wrote: > > > If this is only needed for tests, then you should make the change in test > > code, > > > not in the production code. > > > > I'm not if this is only needed for tests. It was one of the browser tests that > > triggered the problem (the RenderViewHost at construction was different from > the > > one when the popup was hidden), so I think that it could occur in the wild > > (although I haven't repo it. > > > > So I think this should stay, since it might occur in production. > > As far as I understand, it really shouldn't be possible for the RenderViewHost > to change between construction and when the popup is hidden. The RenderViewHost > should not change when there is no page navigation, and the popup should be > dismissed prior to page navigation. Let's do some more investigation to > understand why the test is failing. After digging through the WebContents code a bunch more, I'm now convinced that this or something like it is indeed necessary, as there is no guarantee that we'll discover that a navigation has happened until after the RenderViewHost might already have been swapped out. If you keep the other navigation in the browser test, the DCHECK will still be hit without this code. It looks like it's also possible for web_contents_->GetRenderViewHost() to return NULL during shutdown. Now, it looks like rather than tracking this state ourselves, we can potentially listen for the notification NOTIFICATION_RENDER_VIEW_HOST_CHANGED. If we get that notification, and the new RenderViewHost is the current one for our WebContents, then we can unregister from the old host and set a boolean not to unregister from the new one. This seems somewhat circuitous and potentially fragile, but maybe it's the right way to go. Alternately, we can do what you were doing in this patch set. The thing that makes me unhappy about this is that it seems like every keyboard listener will have to implement this sort of logic, which kind of sucks. For now though, this is the only class that's a KeyboardListener, so I guess it's ok to implement the solution just within this class. https://chromiumcodereview.appspot.com/12223106/diff/15014/chrome/browser/aut... File chrome/browser/autofill/autofill_external_delegate_browsertest.cc (left): https://chromiumcodereview.appspot.com/12223106/diff/15014/chrome/browser/aut... chrome/browser/autofill/autofill_external_delegate_browsertest.cc:114: CURRENT_TAB, content::PAGE_TRANSITION_TYPED, false)); Let's leave this navigation, and get rid of the other one. This navigation seems to tickle a more interesting case around the DCHECK. https://chromiumcodereview.appspot.com/12223106/diff/15014/chrome/browser/aut... File chrome/browser/autofill/autofill_external_delegate_browsertest.cc (right): https://chromiumcodereview.appspot.com/12223106/diff/15014/chrome/browser/aut... chrome/browser/autofill/autofill_external_delegate_browsertest.cc:48: const std::vector<int>& autofill_unique_ids) { nit: OVERRIDE
https://codereview.chromium.org/12223106/diff/5007/chrome/browser/autofill/au... File chrome/browser/autofill/autofill_external_delegate.h (right): https://codereview.chromium.org/12223106/diff/5007/chrome/browser/autofill/au... chrome/browser/autofill/autofill_external_delegate.h:191: content::RenderViewHost* registered_keyboard_listener_with_; On 2013/02/15 05:57:55, Ilya Sherman wrote: > On 2013/02/14 06:19:18, Ilya Sherman wrote: > > On 2013/02/13 22:16:04, csharp wrote: > > > On 2013/02/13 21:58:12, Ilya Sherman wrote: > > > > If this is only needed for tests, then you should make the change in test > > > code, > > > > not in the production code. > > > > > > I'm not if this is only needed for tests. It was one of the browser tests > that > > > triggered the problem (the RenderViewHost at construction was different from > > the > > > one when the popup was hidden), so I think that it could occur in the wild > > > (although I haven't repo it. > > > > > > So I think this should stay, since it might occur in production. > > > > As far as I understand, it really shouldn't be possible for the RenderViewHost > > to change between construction and when the popup is hidden. The > RenderViewHost > > should not change when there is no page navigation, and the popup should be > > dismissed prior to page navigation. Let's do some more investigation to > > understand why the test is failing. > > After digging through the WebContents code a bunch more, I'm now convinced that > this or something like it is indeed necessary, as there is no guarantee that > we'll discover that a navigation has happened until after the RenderViewHost > might already have been swapped out. If you keep the other navigation in the > browser test, the DCHECK will still be hit without this code. It looks like > it's also possible for web_contents_->GetRenderViewHost() to return NULL during > shutdown. > > Now, it looks like rather than tracking this state ourselves, we can potentially > listen for the notification NOTIFICATION_RENDER_VIEW_HOST_CHANGED. If we get > that notification, and the new RenderViewHost is the current one for our > WebContents, then we can unregister from the old host and set a boolean not to > unregister from the new one. This seems somewhat circuitous and potentially > fragile, but maybe it's the right way to go. > > Alternately, we can do what you were doing in this patch set. The thing that > makes me unhappy about this is that it seems like every keyboard listener will > have to implement this sort of logic, which kind of sucks. For now though, this > is the only class that's a KeyboardListener, so I guess it's ok to implement the > solution just within this class. Ok, I revert to this handling and file a bug to better handle removing the KeyboardListener. https://codereview.chromium.org/12223106/diff/15014/chrome/browser/autofill/a... File chrome/browser/autofill/autofill_external_delegate_browsertest.cc (left): https://codereview.chromium.org/12223106/diff/15014/chrome/browser/autofill/a... chrome/browser/autofill/autofill_external_delegate_browsertest.cc:114: CURRENT_TAB, content::PAGE_TRANSITION_TYPED, false)); On 2013/02/15 05:57:55, Ilya Sherman wrote: > Let's leave this navigation, and get rid of the other one. This navigation > seems to tickle a more interesting case around the DCHECK. Added this one back and change the blank page to the bookmark page. (This seems to always trigger the weird case where the render switches from under us, so make sure it is handled). https://codereview.chromium.org/12223106/diff/15014/chrome/browser/autofill/a... File chrome/browser/autofill/autofill_external_delegate_browsertest.cc (right): https://codereview.chromium.org/12223106/diff/15014/chrome/browser/autofill/a... chrome/browser/autofill/autofill_external_delegate_browsertest.cc:48: const std::vector<int>& autofill_unique_ids) { On 2013/02/15 05:57:55, Ilya Sherman wrote: > nit: OVERRIDE Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/csharp@chromium.org/12223106/24010
Message was sent while issue was closed.
Change committed as 182751
Message was sent while issue was closed.
On 2013/02/15 05:57:54, Ilya Sherman wrote: > Charlie, could you take a look and give your advice on how best to approach > safely unregistering ourselves as a listener from the correct RenderViewHost? > Is there perhaps a way to guarantee that we'll be notified that a navigation is > (definitely) about to happen *prior* to the old RenderViewHost being swapped > out? Can you explain why it needs to be prior? The notifications tend to happen right after the swap. Most earlier navigation events aren't guaranteed to commit (e.g., might be a download). The earliest we could catch it is when we tell the current RVH to run its unload handlers, but I don't think there are any events generated for that. > https://chromiumcodereview.appspot.com/12223106/diff/5007/chrome/browser/auto... > File chrome/browser/autofill/autofill_external_delegate.h (right): > > https://chromiumcodereview.appspot.com/12223106/diff/5007/chrome/browser/auto... > chrome/browser/autofill/autofill_external_delegate.h:191: > content::RenderViewHost* registered_keyboard_listener_with_; > On 2013/02/14 06:19:18, Ilya Sherman wrote: > > On 2013/02/13 22:16:04, csharp wrote: > > > On 2013/02/13 21:58:12, Ilya Sherman wrote: > > > > If this is only needed for tests, then you should make the change in test > > > code, > > > > not in the production code. > > > > > > I'm not if this is only needed for tests. It was one of the browser tests > that > > > triggered the problem (the RenderViewHost at construction was different from > > the > > > one when the popup was hidden), so I think that it could occur in the wild > > > (although I haven't repo it. > > > > > > So I think this should stay, since it might occur in production. > > > > As far as I understand, it really shouldn't be possible for the RenderViewHost > > to change between construction and when the popup is hidden. The > RenderViewHost > > should not change when there is no page navigation, and the popup should be > > dismissed prior to page navigation. Let's do some more investigation to > > understand why the test is failing. > > After digging through the WebContents code a bunch more, I'm now convinced that > this or something like it is indeed necessary, as there is no guarantee that > we'll discover that a navigation has happened until after the RenderViewHost > might already have been swapped out. If you keep the other navigation in the > browser test, the DCHECK will still be hit without this code. It looks like > it's also possible for web_contents_->GetRenderViewHost() to return NULL during > shutdown. > > Now, it looks like rather than tracking this state ourselves, we can potentially > listen for the notification NOTIFICATION_RENDER_VIEW_HOST_CHANGED. If we get > that notification, and the new RenderViewHost is the current one for our > WebContents, then we can unregister from the old host and set a boolean not to > unregister from the new one. This seems somewhat circuitous and potentially > fragile, but maybe it's the right way to go. In general, I think we're trying to use things like WebContentsObserver rather than Notifications these days, right? There's a WebContentsObserver::DidCommitProvisionalLoad, but I suppose you'd have to compare the RVH each time. Otherwise I would have recommended NOTIFICATION_WEB_CONTENTS_SWAPPED, but it appears NOTIFICATION_RENDER_VIEW_HOST_CHANGED is synonymous with that. (Wow, that code needs some cleanup.) I suppose either of those would work. > Alternately, we can do what you were doing in this patch set. The thing that > makes me unhappy about this is that it seems like every keyboard listener will > have to implement this sort of logic, which kind of sucks. For now though, this > is the only class that's a KeyboardListener, so I guess it's ok to implement the > solution just within this class. Either way, the bottom line is that you can't assume a WebContents always has the same RenderViewHost. Whether you keep track of the one you're listening to or react when it changes is your call. Charlie > > https://chromiumcodereview.appspot.com/12223106/diff/15014/chrome/browser/aut... > File chrome/browser/autofill/autofill_external_delegate_browsertest.cc (left): > > https://chromiumcodereview.appspot.com/12223106/diff/15014/chrome/browser/aut... > chrome/browser/autofill/autofill_external_delegate_browsertest.cc:114: > CURRENT_TAB, content::PAGE_TRANSITION_TYPED, false)); > Let's leave this navigation, and get rid of the other one. This navigation > seems to tickle a more interesting case around the DCHECK. > > https://chromiumcodereview.appspot.com/12223106/diff/15014/chrome/browser/aut... > File chrome/browser/autofill/autofill_external_delegate_browsertest.cc (right): > > https://chromiumcodereview.appspot.com/12223106/diff/15014/chrome/browser/aut... > chrome/browser/autofill/autofill_external_delegate_browsertest.cc:48: const > std::vector<int>& autofill_unique_ids) { > nit: OVERRIDE
Message was sent while issue was closed.
On 2013/02/15 21:23:32, creis wrote: > On 2013/02/15 05:57:54, Ilya Sherman wrote: > > Charlie, could you take a look and give your advice on how best to approach > > safely unregistering ourselves as a listener from the correct RenderViewHost? > > Is there perhaps a way to guarantee that we'll be notified that a navigation > is > > (definitely) about to happen *prior* to the old RenderViewHost being swapped > > out? > > Can you explain why it needs to be prior? The notifications tend to happen > right after the swap. Most earlier navigation events aren't guaranteed to > commit (e.g., might be a download). The earliest we could catch it is when we > tell the current RVH to run its unload handlers, but I don't think there are any > events generated for that. It reduces the amount of bookkeeping we need to do if can get notified prior to the swap. Our goal is to (1) dismiss the Autofill popup when a navigation occurs and (2) unregister the popup controller as a listener when it is being dismissed. If we get notified *before* the swap happens, then we can just dismiss the popup, which will then synchronously cause the popup to be removed as a listener. If we get notified *after* the swap happens, we need to separately unregister the popup as a listener, and also make a note that it's been unregistered, so that we don't try to unregister it again when we dismiss the popup (which we'll do immediately after unregistering the listener). I guess another option is to unregister ourselves from the old RVH and register with the new one when the swap occurs, even though we know we're going to immediately unregister from the new one. That reduces our bookkeeping, but feels pretty roundabout and potentially fragile (in that we could possibly get forwarded unexpected and unwanted keyboard events from the new RVH)...
Message was sent while issue was closed.
On 2013/02/15 21:54:58, Ilya Sherman wrote: > On 2013/02/15 21:23:32, creis wrote: > > On 2013/02/15 05:57:54, Ilya Sherman wrote: > > > Charlie, could you take a look and give your advice on how best to approach > > > safely unregistering ourselves as a listener from the correct > RenderViewHost? > > > Is there perhaps a way to guarantee that we'll be notified that a navigation > > is > > > (definitely) about to happen *prior* to the old RenderViewHost being swapped > > > out? > > > > Can you explain why it needs to be prior? The notifications tend to happen > > right after the swap. Most earlier navigation events aren't guaranteed to > > commit (e.g., might be a download). The earliest we could catch it is when we > > tell the current RVH to run its unload handlers, but I don't think there are > any > > events generated for that. > > It reduces the amount of bookkeeping we need to do if can get notified prior to > the swap. Our goal is to (1) dismiss the Autofill popup when a navigation > occurs and (2) unregister the popup controller as a listener when it is being > dismissed. If we get notified *before* the swap happens, then we can just > dismiss the popup, which will then synchronously cause the popup to be removed > as a listener. If we get notified *after* the swap happens, we need to > separately unregister the popup as a listener, and also make a note that it's > been unregistered, so that we don't try to unregister it again when we dismiss > the popup (which we'll do immediately after unregistering the listener). > > I guess another option is to unregister ourselves from the old RVH and register > with the new one when the swap occurs, even though we know we're going to > immediately unregister from the new one. That reduces our bookkeeping, but > feels pretty roundabout and potentially fragile (in that we could possibly get > forwarded unexpected and unwanted keyboard events from the new RVH)... Yeah, I don't think I'd recommend changing your underlying RVH unless you expect the listener to operate on the new one. And at least at the moment, I don't know of a way to get notified just before the swap. I'm not super familiar with this feature, but is it reasonable for it to go away if any provisional load starts? That might be easier. Otherwise, I'd probably recommend just doing the extra bookkeeping. |
