Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1941363002:
PasswordController should give up on WebState destruction (Closed)
|
Created:
4 years, 7 months ago by vabr (Chromium) Modified:
4 years, 7 months ago Reviewers:
Eugene But (OOO till 7-30) CC:
chromium-reviews, gcasto+watchlist_chromium.org, mkwst+watchlist-passwords_chromium.org, vabr+watchlistpasswordmanager_chromium.org, sdefresne+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionPasswordController should give up on WebState destruction
The associated bug has a crash where a WebStateObserverBridge method of
PasswordController dereferences a null pointer. The block where the dereference
happens is:
if (weakSelf) {
weakSelf.get()->passwordManager_->OnPasswordFormSubmitted(
weakSelf.get()->passwordManagerDriver_.get(), form);
}
It is likely that between the call to
-webState:didSubmitDocumentWithFormNamed:userInitiated: and the execution
of the block the WebState has been destroyed, and therefore also
passwordManager_ and passwordManagerDriver_ nulled.
This CL adds a BOOL to keep track of the destruction of the WebState, allowing
the controller to give up when the WS is destroyed.
R=eugenebut@chromium.org
BUG=608090
Committed: https://crrev.com/0e96a24352e1ed49716b86177e65b09069ed05a2
Cr-Commit-Position: refs/heads/master@{#391511}
Patch Set 1 #Patch Set 2 : Keep webState separately from the bridge #Patch Set 3 : Fix compile #Patch Set 4 : Just rebased #Patch Set 5 : isBeingDestroyed #
Total comments: 12
Patch Set 6 : Just rebased #Patch Set 7 : Comments addressed #Messages
Total messages: 33 (13 generated)
The CQ bit was checked by vabr@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1941363002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1941363002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Keep webState separately from the bridge
The CQ bit was checked by vabr@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1941363002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1941363002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
Fix compile
The CQ bit was checked by vabr@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1941363002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1941363002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from
==========
PasswordController initialization: WebStateObserverBridge to the end
The associated bug has a crash where a WebStateObserverBridge method of
PasswordController dereferences a null pointer. The block where the dereference
happens is:
if (weakSelf) {
weakSelf.get()->passwordManager_->OnPasswordFormSubmitted(
weakSelf.get()->passwordManagerDriver_.get(), form);
}
It seems like either passwordManager_ or passwordManagerDriver_ must be null.
Both are set to non-null in PasswordController's -initWithWebState, and only
reset again in -detach. So the only moment in the code when PasswordController
could be listening to WebStateObserver methods and have some of those two
pointers null is during -initWithWebState, after the WebStateObserverBridge
creation but before the pointer initialisation. I'm not sure how it could
happen that the -initWithWebState would be interrupted, but currently see no
other explanation.
This CL changes the initialisation order to only create the bridge once the two
pointers are non-null.
R=eugenebut@chromium.org
BUG=608090
==========
to
==========
PasswordController initialization: WebStateObserverBridge to the end
The associated bug has a crash where a WebStateObserverBridge method of
PasswordController dereferences a null pointer. The block where the dereference
happens is:
if (weakSelf) {
weakSelf.get()->passwordManager_->OnPasswordFormSubmitted(
weakSelf.get()->passwordManagerDriver_.get(), form);
}
It seems like either passwordManager_ or passwordManagerDriver_ must be null.
Both are set to non-null in PasswordController's -initWithWebState, and only
reset again in -detach. So the only moment in the code when PasswordController
could be listening to WebStateObserver methods and have some of those two
pointers null is during -initWithWebState, after the WebStateObserverBridge
creation but before the pointer initialisation. I'm not sure how it could
happen that the -initWithWebState would be interrupted, but currently see no
other explanation.
This CL changes the initialisation order to only create the bridge once the two
pointers are non-null. It also adds a weak pointer to the WebState in the
PasswordController. So far, the WebState was accessed through the
WebStateObserverBridge, but because the bridge is now only available late in
the initialization, it is needed to provide the WebState separately (this
was made clear by failing tests).
R=eugenebut@chromium.org
BUG=608090
==========
Hi Eugene, Could you PTAL? I ran the downstream smoke bot on this and it is green. Cheers, Vaclav
The block where app crashes is called asynchronously and can be called after detach. I think the right fix would be to add isBeingDestroyed ivar which is set to YES in |webStateDestroyed:|.
Just rebased
isBeingDestroyed
Description was changed from
==========
PasswordController initialization: WebStateObserverBridge to the end
The associated bug has a crash where a WebStateObserverBridge method of
PasswordController dereferences a null pointer. The block where the dereference
happens is:
if (weakSelf) {
weakSelf.get()->passwordManager_->OnPasswordFormSubmitted(
weakSelf.get()->passwordManagerDriver_.get(), form);
}
It seems like either passwordManager_ or passwordManagerDriver_ must be null.
Both are set to non-null in PasswordController's -initWithWebState, and only
reset again in -detach. So the only moment in the code when PasswordController
could be listening to WebStateObserver methods and have some of those two
pointers null is during -initWithWebState, after the WebStateObserverBridge
creation but before the pointer initialisation. I'm not sure how it could
happen that the -initWithWebState would be interrupted, but currently see no
other explanation.
This CL changes the initialisation order to only create the bridge once the two
pointers are non-null. It also adds a weak pointer to the WebState in the
PasswordController. So far, the WebState was accessed through the
WebStateObserverBridge, but because the bridge is now only available late in
the initialization, it is needed to provide the WebState separately (this
was made clear by failing tests).
R=eugenebut@chromium.org
BUG=608090
==========
to
==========
PasswordController initialization: WebStateObserverBridge to the end
The associated bug has a crash where a WebStateObserverBridge method of
PasswordController dereferences a null pointer. The block where the dereference
happens is:
if (weakSelf) {
weakSelf.get()->passwordManager_->OnPasswordFormSubmitted(
weakSelf.get()->passwordManagerDriver_.get(), form);
}
It seems like either passwordManager_ or passwordManagerDriver_ must be null.
Both are set to non-null in PasswordController's -initWithWebState, and only
reset again in -detach. So one moment in the code when PasswordController
could be listening to WebStateObserver methods and have some of those two
pointers null is during -initWithWebState, after the WebStateObserverBridge
creation but before the pointer initialisation. I'm not sure how it could
happen that the -initWithWebState would be interrupted, but currently see no
other explanation.
The other moment is after -detach and before destruction (note that the block
can be executed with a sufficient delay to fit into that).
This CL changes the initialisation order to only create the bridge once the two
pointers are non-null. It also adds a weak pointer to the WebState in the
PasswordController. So far, the WebState was accessed through the
WebStateObserverBridge, but because the bridge is now only available late in
the initialization, it is needed to provide the WebState separately (this
was made clear by failing tests).
The CL also adds and ivar to mark the availability of the WebState, and cancels
the block execution if the controller is still alive but the WebState no longer
available.
R=eugenebut@chromium.org
BUG=608090
==========
Thanks, Eugene. PTAL (the downstream smoke bot is green as well). I was not sure whether to only have the suggested isBeingDestroyed, or whether to keep the different order of initialization as well. If you have a strong opinion, do let me know. So far I kept both. Cheers, Vaclav
Please update CL comment to reflect the actual changes. lgtm, so you are not blocked by timezone difference https://codereview.chromium.org/1941363002/diff/80001/ios/chrome/browser/pass... File ios/chrome/browser/passwords/password_controller.h (right): https://codereview.chromium.org/1941363002/diff/80001/ios/chrome/browser/pass... ios/chrome/browser/passwords/password_controller.h:52: @property(readonly) BOOL isBeingDestroyed; Does this need to be public? https://codereview.chromium.org/1941363002/diff/80001/ios/chrome/browser/pass... File ios/chrome/browser/passwords/password_controller.mm (right): https://codereview.chromium.org/1941363002/diff/80001/ios/chrome/browser/pass... ios/chrome/browser/passwords/password_controller.mm:246: @synthesize isBeingDestroyed = isBeingDestroyed_; Sorry the name I suggested was not very good. isWebStateDestroyed sounds more correct. https://codereview.chromium.org/1941363002/diff/80001/ios/chrome/browser/pass... ios/chrome/browser/passwords/password_controller.mm:263: isBeingDestroyed_ = NO; From Objective-C Style Guide: Don't initialize instance variables to 0 or nil in the init method; doing so is redundant. https://engdoc.corp.google.com/eng/doc/objcguide.xml?showone=Initialization&c... https://codereview.chromium.org/1941363002/diff/80001/ios/chrome/browser/pass... ios/chrome/browser/passwords/password_controller.mm:264: webState_ = webState; This change is not part of the bugfix and because of that I would limit this CL to have only the fix. This CL may be cherrypicked to release branch so we should limit the amount of changes on branches. https://codereview.chromium.org/1941363002/diff/80001/ios/chrome/browser/pass... ios/chrome/browser/passwords/password_controller.mm:285: webStateObserverBridge_.reset( This also not a part of the fix, so please exclude this from CL. https://codereview.chromium.org/1941363002/diff/80001/ios/chrome/browser/pass... ios/chrome/browser/passwords/password_controller.mm:391: if (weakSelf && [weakSelf isBeingDestroyed] == NO) { NIT: ![weakSelf isBeingDestroyed]
Just rebased
Comments addressed
Description was changed from
==========
PasswordController initialization: WebStateObserverBridge to the end
The associated bug has a crash where a WebStateObserverBridge method of
PasswordController dereferences a null pointer. The block where the dereference
happens is:
if (weakSelf) {
weakSelf.get()->passwordManager_->OnPasswordFormSubmitted(
weakSelf.get()->passwordManagerDriver_.get(), form);
}
It seems like either passwordManager_ or passwordManagerDriver_ must be null.
Both are set to non-null in PasswordController's -initWithWebState, and only
reset again in -detach. So one moment in the code when PasswordController
could be listening to WebStateObserver methods and have some of those two
pointers null is during -initWithWebState, after the WebStateObserverBridge
creation but before the pointer initialisation. I'm not sure how it could
happen that the -initWithWebState would be interrupted, but currently see no
other explanation.
The other moment is after -detach and before destruction (note that the block
can be executed with a sufficient delay to fit into that).
This CL changes the initialisation order to only create the bridge once the two
pointers are non-null. It also adds a weak pointer to the WebState in the
PasswordController. So far, the WebState was accessed through the
WebStateObserverBridge, but because the bridge is now only available late in
the initialization, it is needed to provide the WebState separately (this
was made clear by failing tests).
The CL also adds and ivar to mark the availability of the WebState, and cancels
the block execution if the controller is still alive but the WebState no longer
available.
R=eugenebut@chromium.org
BUG=608090
==========
to
==========
PasswordController should give up on WebState destruction
The associated bug has a crash where a WebStateObserverBridge method of
PasswordController dereferences a null pointer. The block where the dereference
happens is:
if (weakSelf) {
weakSelf.get()->passwordManager_->OnPasswordFormSubmitted(
weakSelf.get()->passwordManagerDriver_.get(), form);
}
It is likely that between the call to
-webState:didSubmitDocumentWithFormNamed:userInitiated: and the execution
of the block the WebState has been destroyed, and therefore also
passwordManager_ and passwordManagerDriver_ nulled.
This CL adds a BOOL to keep track of the destruction of the WebState, allowing
the controller to give up when the WS is destroyed.
R=eugenebut@chromium.org
BUG=608090
==========
Thanks, Eugene! Comments addressed, and the downstream smoke test passed as well, so I'm sending this to the CQ. Cheers, Vaclav https://codereview.chromium.org/1941363002/diff/80001/ios/chrome/browser/pass... File ios/chrome/browser/passwords/password_controller.h (right): https://codereview.chromium.org/1941363002/diff/80001/ios/chrome/browser/pass... ios/chrome/browser/passwords/password_controller.h:52: @property(readonly) BOOL isBeingDestroyed; On 2016/05/03 17:49:49, Eugene But wrote: > Does this need to be public? No, good point. I moved it to the .mm file. https://codereview.chromium.org/1941363002/diff/80001/ios/chrome/browser/pass... File ios/chrome/browser/passwords/password_controller.mm (right): https://codereview.chromium.org/1941363002/diff/80001/ios/chrome/browser/pass... ios/chrome/browser/passwords/password_controller.mm:246: @synthesize isBeingDestroyed = isBeingDestroyed_; On 2016/05/03 17:49:49, Eugene But wrote: > Sorry the name I suggested was not very good. isWebStateDestroyed sounds more > correct. Done. https://codereview.chromium.org/1941363002/diff/80001/ios/chrome/browser/pass... ios/chrome/browser/passwords/password_controller.mm:263: isBeingDestroyed_ = NO; On 2016/05/03 17:49:49, Eugene But wrote: > From Objective-C Style Guide: > > Don't initialize instance variables to 0 or nil in the init method; doing so is > redundant. > > https://engdoc.corp.google.com/eng/doc/objcguide.xml?showone=Initialization&c... Done. https://codereview.chromium.org/1941363002/diff/80001/ios/chrome/browser/pass... ios/chrome/browser/passwords/password_controller.mm:264: webState_ = webState; On 2016/05/03 17:49:49, Eugene But wrote: > This change is not part of the bugfix and because of that I would limit this CL > to have only the fix. > This CL may be cherrypicked to release branch so we should limit the amount of > changes on branches. Done. https://codereview.chromium.org/1941363002/diff/80001/ios/chrome/browser/pass... ios/chrome/browser/passwords/password_controller.mm:285: webStateObserverBridge_.reset( On 2016/05/03 17:49:49, Eugene But wrote: > This also not a part of the fix, so please exclude this from CL. Done. https://codereview.chromium.org/1941363002/diff/80001/ios/chrome/browser/pass... ios/chrome/browser/passwords/password_controller.mm:391: if (weakSelf && [weakSelf isBeingDestroyed] == NO) { On 2016/05/03 17:49:49, Eugene But wrote: > NIT: ![weakSelf isBeingDestroyed] Done.
The CQ bit was checked by vabr@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from eugenebut@chromium.org Link to the patchset: https://codereview.chromium.org/1941363002/#ps120001 (title: "Comments addressed")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1941363002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1941363002/120001
Message was sent while issue was closed.
Description was changed from
==========
PasswordController should give up on WebState destruction
The associated bug has a crash where a WebStateObserverBridge method of
PasswordController dereferences a null pointer. The block where the dereference
happens is:
if (weakSelf) {
weakSelf.get()->passwordManager_->OnPasswordFormSubmitted(
weakSelf.get()->passwordManagerDriver_.get(), form);
}
It is likely that between the call to
-webState:didSubmitDocumentWithFormNamed:userInitiated: and the execution
of the block the WebState has been destroyed, and therefore also
passwordManager_ and passwordManagerDriver_ nulled.
This CL adds a BOOL to keep track of the destruction of the WebState, allowing
the controller to give up when the WS is destroyed.
R=eugenebut@chromium.org
BUG=608090
==========
to
==========
PasswordController should give up on WebState destruction
The associated bug has a crash where a WebStateObserverBridge method of
PasswordController dereferences a null pointer. The block where the dereference
happens is:
if (weakSelf) {
weakSelf.get()->passwordManager_->OnPasswordFormSubmitted(
weakSelf.get()->passwordManagerDriver_.get(), form);
}
It is likely that between the call to
-webState:didSubmitDocumentWithFormNamed:userInitiated: and the execution
of the block the WebState has been destroyed, and therefore also
passwordManager_ and passwordManagerDriver_ nulled.
This CL adds a BOOL to keep track of the destruction of the WebState, allowing
the controller to give up when the WS is destroyed.
R=eugenebut@chromium.org
BUG=608090
==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from
==========
PasswordController should give up on WebState destruction
The associated bug has a crash where a WebStateObserverBridge method of
PasswordController dereferences a null pointer. The block where the dereference
happens is:
if (weakSelf) {
weakSelf.get()->passwordManager_->OnPasswordFormSubmitted(
weakSelf.get()->passwordManagerDriver_.get(), form);
}
It is likely that between the call to
-webState:didSubmitDocumentWithFormNamed:userInitiated: and the execution
of the block the WebState has been destroyed, and therefore also
passwordManager_ and passwordManagerDriver_ nulled.
This CL adds a BOOL to keep track of the destruction of the WebState, allowing
the controller to give up when the WS is destroyed.
R=eugenebut@chromium.org
BUG=608090
==========
to
==========
PasswordController should give up on WebState destruction
The associated bug has a crash where a WebStateObserverBridge method of
PasswordController dereferences a null pointer. The block where the dereference
happens is:
if (weakSelf) {
weakSelf.get()->passwordManager_->OnPasswordFormSubmitted(
weakSelf.get()->passwordManagerDriver_.get(), form);
}
It is likely that between the call to
-webState:didSubmitDocumentWithFormNamed:userInitiated: and the execution
of the block the WebState has been destroyed, and therefore also
passwordManager_ and passwordManagerDriver_ nulled.
This CL adds a BOOL to keep track of the destruction of the WebState, allowing
the controller to give up when the WS is destroyed.
R=eugenebut@chromium.org
BUG=608090
Committed: https://crrev.com/0e96a24352e1ed49716b86177e65b09069ed05a2
Cr-Commit-Position: refs/heads/master@{#391511}
==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/0e96a24352e1ed49716b86177e65b09069ed05a2 Cr-Commit-Position: refs/heads/master@{#391511} |
