Do you have a crash stack, Pink?

olivierrobin can you ptal?

Yadira can you confirm if this repro's on 72 or not? Also check if this happens on 73 for all device configs?

I am not able to reproduce this crash.

Chrome Beta 72.0.3626.28, Chrome Canary 73.0.3645.0
iPhone XS iOS12, iPhone 8 iOS12
https://drive.google.com/open?id=1iT8QIxi7TxSzs-RFFwGDY8xDSzULyv5G 

I just ran into this crash on my iPhoneX as well. Also able to reproduce using the steps from comment#0 on a iPhone7plus.
But when I looked about://crashes on both the devices, the crash stacktraces didn't match on from both devices.
Crash id from iPhoneX, http://crash/7f5caf383a198f55
Crash id from iPhone7Plus, http://crash/c8f05a9500d5fc1d

I will look more into it tomorrow and update if this happening in M72 or not.

I could reproduce on M73 but not on M72.
This seems to be related to the chrome-broadcaster.

Perhaps unrelated, but c#3 crash http://crash/c8f05a9500d5fc1d looks to be a snapshot bug that edchin@ is already looking at.

We investigated more with gambard@ and here are our conclusions:

1 Crash happens because |application:performActionForShortcutItem:| which handles the 3d touch action is called after the initial UI is created. This means that when initial UI is created, we consider that the main BVC is considered as the main one, and we switch it immediately to OTR BVC. On the main BVC, there is a call to willAppear:, but not to didAppear: and willDisappear.
Result is that StartBroadcastingToolbarUI is called on mainBVC, but not StopBroadcastingToolbarUI, and as a consequence, we send messages to released pointers.

2 A first solution would be to check in |application:didFinishLaunching:| if the BVC to activate is incognito or not. This means that in the case we discuss here the main BVC will not be presented and this particular bug will not happen. But this cause a second issue:
  - on a cold start in main BVC, if no tab is present, a new one is created. At the moment, every cold start is on mainBVC, so on Cold start, there is always a tab in mainBVC.
  - After this fix, if you cold start in incognito, it is possible that main BVC does not contain any tab. If the restore toolbar must be displayed, it is not attached to any tab and it crashes.

3 So it is possible to create a background tab in the main tab model on every cold start. This has to be done during the tabModel creation, otherwise observer will crash because the tabModel is not active.
This also require to present the toolbar on the first tab if no tab is active.

I did a POC CL (https://chromium-review.googlesource.com/c/chromium/src/+/1386851) that does these changes (CL is not ready, only a proof of concept).

Other option would be to find the point of regression and revert it, but I think the current behavior is wrong anyway.

Adding kkhorimoto who worked on ChromeBroadcaster, marq who knows the staring sequence of BVC better than me.
WDYT?

Issue 915123 has been merged into this issue.

Thanks for the investigation, Olivier and Gauthier!  I've merged Issue 915123 into this bug because (1) from c#6 seems to describe the situation pretty well.  UIViewController presentation can be cancelled, so it was wrong for us to assume that |-viewWillAppear:| will always be followed by the other view appearance selectors.  I think that maybe updating |-updateBroadcastState| to check self.viewVisible rather than self.visible might fix this issue. |viewVisible| is set in |-viewDidAppear:|, so we won't run into the issue where we don't receive the corresponding |-viewWillDisappear:| to stop broadcasting.  Also, I think it makes more sense to begin broadcasting at this point since it doesn't make sense to broadcast UI information while a transition animation is occurring (especially since the broadcaster is only used for fullscreen, and no scrolling is occurring).  If an object cares about UI state during the animation, it should be using the transition coordinator.

Not sure about what the best solution for the infobar issue is though.  If the user opens a new incognito tab using force touch, but the app crashed before, do we still want to show the crash recovery infobar?  It seems like a strange user flow to open an incognito tab and browse there, but later go to the main BVC and restore the crashed state.  Would it make more sense to display the infobar over the newly opened incognito tab (maybe with different messaging related to only restoring non-incognito tabs from your previous session)?  If we want to maintain the current behavior of displaying the crash recovery infobar over the first non-incognito tab opened after a crash, then I think that manually inserting the tab may be the correct option.  However, I think that this should occur from the crash restore helper and not be baked into TabModel.  I can't tell for sure just by looking, but it seems like your POC CL might break the new tab animation that occurs when opening in a non-crash-recovery scenario when there aren't any tabs in the main BVC.

Again, thanks for the investigation! I was pretty puzzled at how the broadcaster got into the corrupt state, and hadn't even considered that the BVC would be hidden without calling |-viewWillDisappear:|.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e7d6b922f16454d67ca07e0230c85b697d2dab24

commit e7d6b922f16454d67ca07e0230c85b697d2dab24
Author: Kurt Horimoto <kkhorimoto@chromium.org>
Date: Fri Dec 21 23:06:32 2018

[iOS] Start broadcasting when BVC's view is visible.

BVC previously used self.visible, which is set in |-viewWillAppear:|, as
a trigger to start broadcasting its UI state.  However, cancelled
UIViewController transitions can sometimes result in |-viewWillAppear:|
being called without its accompanying |-viewDidAppear:| and
|-viewWillDisappear:| selectors.  As a result, BVC continued to
broadcast its UI even if its views were removed from the hierarchy and
deallocated, thus resulting in the crash from crbug.com/915123.

This CL updates BVC.broadcasting to be gated on BVC.viewVisible, which
is instead set in |-viewDidAppear:| after a successful presentation.
In addition to mitigating the crash, this also is an improvement
semantically, as BVC should not be broadcasting its state until it is
fully presented.  If objects care about BVC's UI during a transition,
it should be using the transition coordinator.

Currently, the BVC's broadcasted values are only used by
FullscreenController, and no scrolling is occurring in between
|-viewWillAppear:| and |-viewDidAppear:|, meaning that this CL has
no functional change.

Bug: 915123,  916581 
Change-Id: Ifc0245e6e93f823c733be41ea471954912e7b577
Reviewed-on: https://chromium-review.googlesource.com/c/1388713
Reviewed-by: edchin <edchin@chromium.org>
Reviewed-by: Justin Cohen <justincohen@chromium.org>
Commit-Queue: Kurt Horimoto <kkhorimoto@chromium.org>
Cr-Commit-Position: refs/heads/master@{#618654}
[modify] https://crrev.com/e7d6b922f16454d67ca07e0230c85b697d2dab24/ios/chrome/browser/ui/browser_view_controller.mm

The fact that the broadcast was started in viewWillAppear was a fix for the issue 908796.

Yes, the CL in c#10 was for the broadcaster portion of this crash (i.e. point 1 from c#6).  It does not fix the infobar issue though.

Assigning to kkhorimoto to see if #10 CL fix the issue

The CL from c#10 only fixes the portion of the crash where stemming from the broadcaster not being properly cleaned up due to the unbalanced UIViewController presentation callbacks (i.e. point 1 from c#6).  I haven't done anything for parts 2 and 3 from c#6 though.  Do you want me to review your proof of concept CL, or do you have another approach you want to use for that part?

Unable to repro the crash on the latest M73(73.0.3665.0) canary build
Devices: iPhone X, iPhone 7, iPhone 8
OS Version: 12.0.1

Video: 
https://drive.google.com/open?id=1Iw9yFF_1-HQCix6ninQvJ3sGloXaxAXv

I don't think the CL in #10 will trigger the issues mentioned in 6.
So as long as this does not break cb/908796, I think we are good for this bug.

On a longer term, something may be broken in the current startup sequence, because of the issue mentionned in 6.
The fact that a BVC can be presented without being dismissed may leave some object in a weird state.
We should audit BVC to see the consequence of this viewWillAppear call and see if something should be cleaned.
There may also be consequences in the UIKit code.
Sooner or later the issue will be triggered again because this is an expected scenario.

I think we should change the startup logic so when opening an incognito tab, the incognito BVC is the first to be launched.
We should fix the restore infobar to work in this scenario. The POC CL may hint how we can solve it, but if we are not constraint by the crash, we may do a better fix.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6424558073de16cfa7cf2e446e9a5f5c54b692a8

commit 6424558073de16cfa7cf2e446e9a5f5c54b692a8
Author: Kurt Horimoto <kkhorimoto@chromium.org>
Date: Mon Jan 14 22:09:03 2019

[iOS] Start broadcasting when BVC's view is visible.

BVC previously used self.visible, which is set in |-viewWillAppear:|, as
a trigger to start broadcasting its UI state.  However, cancelled
UIViewController transitions can sometimes result in |-viewWillAppear:|
being called without its accompanying |-viewDidAppear:| and
|-viewWillDisappear:| selectors.  As a result, BVC continued to
broadcast its UI even if its views were removed from the hierarchy and
deallocated, thus resulting in the crash from crbug.com/915123.

This CL updates BVC.broadcasting to be gated on BVC.viewVisible, which
is instead set in |-viewDidAppear:| after a successful presentation.
In addition to mitigating the crash, this also is an improvement
semantically, as BVC should not be broadcasting its state until it is
fully presented.  If objects care about BVC's UI during a transition,
it should be using the transition coordinator.

Currently, the BVC's broadcasted values are only used by
FullscreenController, and no scrolling is occurring in between
|-viewWillAppear:| and |-viewDidAppear:|, meaning that this CL has
no functional change.

Bug: 915123,  916581 
Change-Id: Ifc0245e6e93f823c733be41ea471954912e7b577
Reviewed-on: https://chromium-review.googlesource.com/c/1388713
Reviewed-by: edchin <edchin@chromium.org>
Reviewed-by: Justin Cohen <justincohen@chromium.org>
Commit-Queue: Kurt Horimoto <kkhorimoto@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#618654}(cherry picked from commit e7d6b922f16454d67ca07e0230c85b697d2dab24)
Reviewed-on: https://chromium-review.googlesource.com/c/1410009
Reviewed-by: Kurt Horimoto <kkhorimoto@chromium.org>
Cr-Commit-Position: refs/branch-heads/3626@{#679}
Cr-Branched-From: d897fb137fbaaa9355c0c93124cc048824eb1e65-refs/heads/master@{#612437}
[modify] https://crrev.com/6424558073de16cfa7cf2e446e9a5f5c54b692a8/ios/chrome/browser/ui/browser_view_controller.mm

The following revision refers to this bug: 
https://chromium.googlesource.com/chromium/src.git/+/6424558073de16cfa7cf2e446e9a5f5c54b692a8

Commit: 6424558073de16cfa7cf2e446e9a5f5c54b692a8
Author: kkhorimoto@chromium.org
Commiter: kkhorimoto@chromium.org
Date: 2019-01-14 22:09:03 +0000 UTC

[iOS] Start broadcasting when BVC's view is visible.

BVC previously used self.visible, which is set in |-viewWillAppear:|, as
a trigger to start broadcasting its UI state.  However, cancelled
UIViewController transitions can sometimes result in |-viewWillAppear:|
being called without its accompanying |-viewDidAppear:| and
|-viewWillDisappear:| selectors.  As a result, BVC continued to
broadcast its UI even if its views were removed from the hierarchy and
deallocated, thus resulting in the crash from crbug.com/915123.

This CL updates BVC.broadcasting to be gated on BVC.viewVisible, which
is instead set in |-viewDidAppear:| after a successful presentation.
In addition to mitigating the crash, this also is an improvement
semantically, as BVC should not be broadcasting its state until it is
fully presented.  If objects care about BVC's UI during a transition,
it should be using the transition coordinator.

Currently, the BVC's broadcasted values are only used by
FullscreenController, and no scrolling is occurring in between
|-viewWillAppear:| and |-viewDidAppear:|, meaning that this CL has
no functional change.

Bug: 915123,  916581 
Change-Id: Ifc0245e6e93f823c733be41ea471954912e7b577
Reviewed-on: https://chromium-review.googlesource.com/c/1388713
Reviewed-by: edchin <edchin@chromium.org>
Reviewed-by: Justin Cohen <justincohen@chromium.org>
Commit-Queue: Kurt Horimoto <kkhorimoto@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#618654}(cherry picked from commit e7d6b922f16454d67ca07e0230c85b697d2dab24)
Reviewed-on: https://chromium-review.googlesource.com/c/1410009
Reviewed-by: Kurt Horimoto <kkhorimoto@chromium.org>
Cr-Commit-Position: refs/branch-heads/3626@{#679}
Cr-Branched-From: d897fb137fbaaa9355c0c93124cc048824eb1e65-refs/heads/master@{#612437}

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/440e7ddc051c55e446f79d0ef7e9f463148983f2

commit 440e7ddc051c55e446f79d0ef7e9f463148983f2
Author: Kurt Horimoto <kkhorimoto@chromium.org>
Date: Fri Jan 18 00:33:36 2019

Revert "[iOS] Start broadcasting when BVC's view is visible."

This reverts commit 6424558073de16cfa7cf2e446e9a5f5c54b692a8.

Reason for revert: <INSERT REASONING HERE>

Original change's description:
> [iOS] Start broadcasting when BVC's view is visible.
> 
> BVC previously used self.visible, which is set in |-viewWillAppear:|, as
> a trigger to start broadcasting its UI state.  However, cancelled
> UIViewController transitions can sometimes result in |-viewWillAppear:|
> being called without its accompanying |-viewDidAppear:| and
> |-viewWillDisappear:| selectors.  As a result, BVC continued to
> broadcast its UI even if its views were removed from the hierarchy and
> deallocated, thus resulting in the crash from crbug.com/915123.
> 
> This CL updates BVC.broadcasting to be gated on BVC.viewVisible, which
> is instead set in |-viewDidAppear:| after a successful presentation.
> In addition to mitigating the crash, this also is an improvement
> semantically, as BVC should not be broadcasting its state until it is
> fully presented.  If objects care about BVC's UI during a transition,
> it should be using the transition coordinator.
> 
> Currently, the BVC's broadcasted values are only used by
> FullscreenController, and no scrolling is occurring in between
> |-viewWillAppear:| and |-viewDidAppear:|, meaning that this CL has
> no functional change.
> 
> Bug: 915123,  916581 
> Change-Id: Ifc0245e6e93f823c733be41ea471954912e7b577
> Reviewed-on: https://chromium-review.googlesource.com/c/1388713
> Reviewed-by: edchin <edchin@chromium.org>
> Reviewed-by: Justin Cohen <justincohen@chromium.org>
> Commit-Queue: Kurt Horimoto <kkhorimoto@chromium.org>
> Cr-Original-Commit-Position: refs/heads/master@{#618654}(cherry picked from commit e7d6b922f16454d67ca07e0230c85b697d2dab24)
> Reviewed-on: https://chromium-review.googlesource.com/c/1410009
> Reviewed-by: Kurt Horimoto <kkhorimoto@chromium.org>
> Cr-Commit-Position: refs/branch-heads/3626@{#679}
> Cr-Branched-From: d897fb137fbaaa9355c0c93124cc048824eb1e65-refs/heads/master@{#612437}

TBR=kkhorimoto@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 915123,  916581 
Change-Id: I9d25ea2f675e7944635c95538cf808cc7877f7bc
Reviewed-on: https://chromium-review.googlesource.com/c/1419129
Reviewed-by: Kurt Horimoto <kkhorimoto@chromium.org>
Cr-Commit-Position: refs/branch-heads/3626@{#719}
Cr-Branched-From: d897fb137fbaaa9355c0c93124cc048824eb1e65-refs/heads/master@{#612437}
[modify] https://crrev.com/440e7ddc051c55e446f79d0ef7e9f463148983f2/ios/chrome/browser/ui/browser_view_controller.mm

The following revision refers to this bug: 
https://chromium.googlesource.com/chromium/src.git/+/440e7ddc051c55e446f79d0ef7e9f463148983f2

Commit: 440e7ddc051c55e446f79d0ef7e9f463148983f2
Author: kkhorimoto@chromium.org
Commiter: kkhorimoto@chromium.org
Date: 2019-01-18 00:33:36 +0000 UTC

Revert "[iOS] Start broadcasting when BVC's view is visible."

This reverts commit 6424558073de16cfa7cf2e446e9a5f5c54b692a8.

Reason for revert: <INSERT REASONING HERE>

Original change's description:
> [iOS] Start broadcasting when BVC's view is visible.
> 
> BVC previously used self.visible, which is set in |-viewWillAppear:|, as
> a trigger to start broadcasting its UI state.  However, cancelled
> UIViewController transitions can sometimes result in |-viewWillAppear:|
> being called without its accompanying |-viewDidAppear:| and
> |-viewWillDisappear:| selectors.  As a result, BVC continued to
> broadcast its UI even if its views were removed from the hierarchy and
> deallocated, thus resulting in the crash from crbug.com/915123.
> 
> This CL updates BVC.broadcasting to be gated on BVC.viewVisible, which
> is instead set in |-viewDidAppear:| after a successful presentation.
> In addition to mitigating the crash, this also is an improvement
> semantically, as BVC should not be broadcasting its state until it is
> fully presented.  If objects care about BVC's UI during a transition,
> it should be using the transition coordinator.
> 
> Currently, the BVC's broadcasted values are only used by
> FullscreenController, and no scrolling is occurring in between
> |-viewWillAppear:| and |-viewDidAppear:|, meaning that this CL has
> no functional change.
> 
> Bug: 915123,  916581 
> Change-Id: Ifc0245e6e93f823c733be41ea471954912e7b577
> Reviewed-on: https://chromium-review.googlesource.com/c/1388713
> Reviewed-by: edchin <edchin@chromium.org>
> Reviewed-by: Justin Cohen <justincohen@chromium.org>
> Commit-Queue: Kurt Horimoto <kkhorimoto@chromium.org>
> Cr-Original-Commit-Position: refs/heads/master@{#618654}(cherry picked from commit e7d6b922f16454d67ca07e0230c85b697d2dab24)
> Reviewed-on: https://chromium-review.googlesource.com/c/1410009
> Reviewed-by: Kurt Horimoto <kkhorimoto@chromium.org>
> Cr-Commit-Position: refs/branch-heads/3626@{#679}
> Cr-Branched-From: d897fb137fbaaa9355c0c93124cc048824eb1e65-refs/heads/master@{#612437}

TBR=kkhorimoto@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 915123,  916581 
Change-Id: I9d25ea2f675e7944635c95538cf808cc7877f7bc
Reviewed-on: https://chromium-review.googlesource.com/c/1419129
Reviewed-by: Kurt Horimoto <kkhorimoto@chromium.org>
Cr-Commit-Position: refs/branch-heads/3626@{#719}
Cr-Branched-From: d897fb137fbaaa9355c0c93124cc048824eb1e65-refs/heads/master@{#612437}

Hey Olivier, do you have repro steps for the infobar-related portion of the crash you described in c#6?  I might mark this as Fixed unless you can still reproduce that case.

Can't repro on latest M73 canary.