Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2949673002:
Settings EG Test Autorelease Fix
DescriptionSettings EG Test Autorelease Fix
Moving other code to ARC (https://codereview.chromium.org/2916473002/)
has caused some changes in what objects are autoreleased. This can
cause objects to live longer than some tests expect and trip DCHECKs.
This change uses an autoreleasepool to ensure that autoreleased
objects are no longer in a pool when the test comes to an end.
An alternative would be to remove the failing DCHECK altogether as it
is debateable as to whether issues like this should be worked around
at all.
I have run all of the other tests in ios_chrome_settings_egtests_module
with success.
BUG=None
Test=None
Patch Set 1 #Patch Set 2 : Alternative fix #Patch Set 3 : Remove DCHECKs #Messages
Total messages: 29 (12 generated)
Description was changed from ========== Settings EG Test Autorelease Fix Moving other code to ARC (https://codereview.chromium.org/2916473002/) has caused some changes in what objects are autoreleased. This can cause objects to live longer than some tests expect and trip DCHECKs. This change uses an autoreleasepool to ensure that autoreleased objects are no longer in a pool when the test comes to an end. An alternative would be to remove the failing DCHECK altogether as it is debateable as to whether issues like this should be worked around at all. BUG=None Test=None ========== to ========== Settings EG Test Autorelease Fix Moving other code to ARC (https://codereview.chromium.org/2916473002/) has caused some changes in what objects are autoreleased. This can cause objects to live longer than some tests expect and trip DCHECKs. This change uses an autoreleasepool to ensure that autoreleased objects are no longer in a pool when the test comes to an end. An alternative would be to remove the failing DCHECK altogether as it is debateable as to whether issues like this should be worked around at all. I have run all of the other tests in ios_chrome_settings_egtests_module with success. BUG=None Test=None ==========
peterlaurens@chromium.org changed reviewers: + gchatz@chromium.org, sdefresne@chromium.org
The CQ bit was checked by peterlaurens@chromium.org 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...
On 2017/06/19 23:01:03, commit-bot: I haz the power wrote: > Dry run: CQ is trying da patch. > > Follow status at: > https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or... This is also failing other EG tests now that I am running them all. There is an alternative fix that does not rely on adding autoreleasepools everywhere, and should still keep the test valid. I will upload a new patch imminently.
On 2017/06/19 23:17:10, PL wrote: > On 2017/06/19 23:01:03, commit-bot: I haz the power wrote: > > Dry run: CQ is trying da patch. > > > > Follow status at: > > > https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or... > > This is also failing other EG tests now that I am running them all. > > There is an alternative fix that does not rely on adding autoreleasepools > everywhere, and should still keep > the test valid. I will upload a new patch imminently. I've uploaded a new patch which relies upon an async check to ensure it runs after the autoreleasepool has been drained for all cases (instead of adding an individual pool for each case that hits this, which could be many). PTAL, and thanks!
peterlaurens@chromium.org changed reviewers: - gchatz@chromium.org
- gchatz Sylvain, I think I need your guidance here. Thank you!
The CQ bit was checked by sdefresne@chromium.org 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...
On 2017/06/23 01:27:10, PL wrote: > - gchatz > > Sylvain, I think I need your guidance here. > > Thank you! Will review once I get the result from the CQ bots.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
I think we should just remove the DCHECK. As discussed there is no guarantee that calling -release on an Objective-C object will lead to -dealloc call. Code depending on that assumption is incorrect as discussed on email. Instead if cleanup has to be done, then -shutdown or -cleanup (or similarly named methods) must be added and called before the -release (i.e. before clearing the scoped_nsobject).
On 2017/06/23 17:03:18, sdefresne wrote: > I think we should just remove the DCHECK. As discussed there is no guarantee > that calling -release on an Objective-C object will lead to -dealloc call. Code > depending on that assumption is incorrect as discussed on email. > > Instead if cleanup has to be done, then -shutdown or -cleanup (or similarly > named methods) must be added and called before the -release (i.e. before > clearing the scoped_nsobject). Thanks Sylvain, I've removed the DCHECKs, this will allow me to resubmit some ARC work that was causing issues here. PTAL! Thanks, - Peter
peterlaurens@chromium.org changed reviewers: + rohitrao@chromium.org
+ Rohit (in case he's available first) Once we have this fixed we can arc-ify ios/web again. Thanks!
rohitrao@chromium.org changed reviewers: + eugenebut@chromium.org
Do you know why why added these DCHECKs in the first place? What happens if configuration/router/pool stay alive past the end of this call? Does that create problems elsewhere in the app? I agree with Sylvain that you can't make any guarantees about the lifetimes of objc objects, but I'd like to understand what this code was trying to protect before we delete it. If there's a privacy or security-related reason why these objects need to go away promptly, then we may need to rework the app to deal with that. +eugenebut
On 2017/06/30 17:16:13, rohitrao (ping after 24h) wrote: > Do you know why why added these DCHECKs in the first place? What happens if > configuration/router/pool stay alive past the end of this call? Does that > create problems elsewhere in the app? > > I agree with Sylvain that you can't make any guarantees about the lifetimes of > objc objects, but I'd like to understand what this code was trying to protect > before we delete it. If there's a privacy or security-related reason why these > objects need to go away promptly, then we may need to rework the app to deal > with that. > > +eugenebut I think the intention here is to have a mechanism to ensure that no other component retains these objects. If we wanted to continue doing so we could use the dispatch_after in patch 2 which would check after the next runloop has run and should be a valid check. I don't know how negative the impact of these objects being alive after a purge is however, Eugene may know more about how critical that is...
> I don't know how negative the impact of these objects being alive after a purge > is however, Eugene may know more about how critical that is... not lgtm. Cookie will not be cleared if WKWebView or WKWebViewConfiguration stays alive, so these DCHECKs are actually needed. It is critical for cookie clearing functionality ta make sure that all web views and configuration objects were destroyed.
On 2017/06/30 17:27:31, Eugene But wrote: > > I don't know how negative the impact of these objects being alive after a > purge > > is however, Eugene may know more about how critical that is... > not lgtm. Cookie will not be cleared if WKWebView or WKWebViewConfiguration > stays alive, so these DCHECKs are actually needed. It is critical for cookie > clearing functionality ta make sure that all web views and configuration objects > were destroyed. Thanks Eugene! What do you think about a solution like patch set 2: https://codereview.chromium.org/2949673002/diff2/1:20001/ios/web/web_state/ui... ... where the checks are dispatch_asynced so the runloop gets a chance to complete once before checking?
On 2017/06/30 17:29:21, PL wrote: > On 2017/06/30 17:27:31, Eugene But wrote: > > > I don't know how negative the impact of these objects being alive after a > > purge > > > is however, Eugene may know more about how critical that is... > > not lgtm. Cookie will not be cleared if WKWebView or WKWebViewConfiguration > > stays alive, so these DCHECKs are actually needed. It is critical for cookie > > clearing functionality ta make sure that all web views and configuration > objects > > were destroyed. > > Thanks Eugene! > > What do you think about a solution like patch set 2: > > https://codereview.chromium.org/2949673002/diff2/1:20001/ios/web/web_state/ui... > > ... where the checks are dispatch_asynced so the runloop gets a chance to > complete once before checking? That's pretty much the same as removing the checks. Configuration object must be killed before cookie clearing, not after. Is it a goal to convert everything to ARC? If so, then we may need to refactor cookie clearing to make sure that all web views and configuration objects are destroyed before calling |removeDataOfTypes:|.
In this case, it seems like the ARC conversion is just exposing the underlying issue. Should this be an asynchronous API that waits until the configuration is destroyed? I don't think there's any way to guarantee that a synchronous API will destroy these objects, even if it happens to work today in practice.
On 2017/06/30 17:50:04, rohitrao (ping after 24h) wrote: > In this case, it seems like the ARC conversion is just exposing the underlying > issue. > > Should this be an asynchronous API that waits until the configuration is > destroyed? I don't think there's any way to guarantee that a synchronous API > will destroy these objects, even if it happens to work today in practice. Async API should work. We can attach associated object to web view configuration and wait until it's dealloc is called. We can do the same thing for all web views. Also we can check if purging web view configuration is critical for cookie clearing on iOS 10 and if it's not we can remove these DCHECKs after dropping iOS9.
Sorry, I was wrong. Purging configuration is needed for clearing visited links, and it is not a prerequisite for cookie clearing. Purging configuration is the last operation during browsing data clearing, so there is no need to wait until WKWebViewConfiguration is deallocated. Please update CL description as it does not look up to date. lgtm.
lgtm Please fix description before landing to say that those DCHECK cannot work with ARC and appears to be unnecessary (as the object -dealloc do nothing) so there is no need to add -shutdown methods.
stkhapugin@chromium.org changed reviewers: + stkhapugin@chromium.org
Since Peter unfortunately can't look at this at the moment, I've started https://chromium-review.googlesource.com/c/569965/ with an up-to-date description. |
