cblume@, according to the bisect the regression was introduced in this CL: crrev.com/c/861643. Could you take a look?

Yep, this is almost certainly my patch.
I'll look into it.

 Issue 840036  has been merged into this issue.

I am unable to repro this locally.

Can you still repro this? If so, are you on M67 (beta)?

I just built and tried tip of tree. It seems like this might already be fixed.

I also can't repro on ToT 68.0.3426.2. Marking as Won't Fix. I'll re-open if it comes back. Thanks!

Reopening: Is this in M67? If so we need to fix it before M67 hits stable.

Well, yes, it does repro in the current M67 beta 67.0.3396.30.

We will need to merge in fixes for M67.

On M67, when there are no devices, the dialog shows a frozen spinner (a dot) instead of help text/link. So we should merge a fix into M67.

I bisected to determine that it is fixed by crrev.com/c/1011556. However, that CL introduces another regression ( issue 840036 ) in which the dialog nondeterministically becomes completely blank. That regression is present in the latest Canary 68.0.3424.0, so it is likely to not have been fixed yet. It would be best if we could merge 1011556 plus a fix to the blanking issue into M67.

However, it might be hard to find a fix for the blanking issue -- I haven't been able to reproduce it on TOT or bisect builds.

cblume@ could you please merge in the CL mentioned ASAP. #9 do you have reliable repro steps in canary. A lot of refactors have happened in M68 so merging back into M67 will not be easy. We might have to do a hot fix for that branch.

I noticed a missing:

client_->SynchronizeVisualProperties() in BrowserCompositorMac::UpdateForAutoResize:

https://cs.chromium.org/chromium/src/content/browser/renderer_host/browser_compositor_view_mac.mm?type=cs&q=UpdateForAutoResize&l=356

That could explain the hang.

Yesterday I also traced it down to the CL mentioned in #9.

The problem is before that patch, calls to OnResizeDueToAutoResize *should* have had local_surface_id_ = parent_local_surface_id_allocator_.UpdateFromChild(). It was updating but never setting the local_surface_id_.

With the patch, local_surface_id_ is removed and we rely on the parent's copy, which was updated.

Reproing isn't reliable because it is a race. You just have to bring up the Cast dialog several time and eventually it will be wrong.


I'll look into that other issue next (and de-dupe if it is separate. It sounds like it is.)

Correction: The CL I narrowed it down to wasn't 1011556. It was crrev.com/c/1024747

I think it is safe to apply that patch to M67 to fix this bug.
And I think it will fix the other bug (duped into this) as well. I am verifying that now.

I'm afraid crrev.com/c/1024747 doesn't fix the other bug, since I'm still able to reproduce it on the current Canary build.

FWIW, I think https://chromium-review.googlesource.com/c/chromium/src/+/1051868/11/content/browser/renderer_host/browser_compositor_view_mac.mm

probably fixes that issue.

cblume@ Please confirm?

I've been trying to find the root cause of these problems (there are more than one). An ASCII graph over time would look like:
___.../\........

From does-not-repro to 1-in-30 runs to a spike up to 1-in-3 runs.

takumif@, I will find and fix the problem you saw. It is one of those "."s in the ASCII graph. But I want to focus on the M67 problem first.


The initial post suspects the CL that began using child allocation. I cannot get this commit to repro. I just ran it 6x30 times (6 instances of Chrome, 30 times bringing up the Cast dialog per Chrome instance).

I then checked out 67.0.3396.30 (mentioned in #7) and tried 6x30 times again. I still could not repro.

Yesterday, 67.0.3396.40 hit the M67 beta. So I checked it out and tried another 6x30 times. It still could not repro.


I'll update this thread tomorrow with more info about the M68+ bug (the "."s in the graph). But for now, I am not so sure we have a M67 problem -- no repros in 540 runs. Maybe I am doing something different from you, dbbrooks@. Or maybe it just doesn't repro on my machine. But I am able to repro the separate bug (same visual result) in M68.

cblume@, thank you for looking into this.

It is interesting that you cannot repro the "dot" bug. On my Mac Pro and the MacBook Air that dbbrooks@ is testing on, it reproduces 100% of the time on the official beta release, 67.0.3396.30. I just updated to 67.0.3396.40 and it still repros 100%.

The bug is the most obvious when you're not logged into the browser (no email address under "Cast to" in the dialog), and you have no Cast devices on the network. Under these conditions the dialog just freezes with the frozen spinner / "dot".

Please let me know if you still can't get a repro.

Chris, David just told me he and you got a repro -- cool.

Yep.

It turns out the top of the Cast dialog has a tutorial with a "OK Got It" button to dismiss. Because I had not yet dismissed this tutorial, my Cast dialog was immediately resizing.

Similarly, when you have dismissed the dialog, having a Cast device on the network causes a resize as well, which fixes the problem.

This gave me a repro case AND a clue (no updates until resize) to investigate further.

Merge Request 67:
I believe we should apply this patch onto 67:
https://chromium-review.googlesource.com/c/chromium/src/+/1013346
More info below.



We have a few options.
I think the best option is to revert:
https://chromium-review.googlesource.com/c/chromium/src/+/1013346
I was unable to gclient sync this because the SHA expected of webrtc isn't found. That means I wasn't able to build and test it. I also couldn't upload the revert. This link is an old revert I accidentally created shortly after landing the original patch.

An alternative option is to attempt a last-minute fix.
We can fix the issue reported in this bug by applying the suppression patch:
https://chromium-review.googlesource.com/c/chromium/src/+/1011556
HOWEVER, that introduces a new bug as reported in  https://crbug.com/840036  which has not yet been fixed.
In addition to that, I believe Eric feels like his Guest patch would also need to be applied as that might also be broken.

So we would either:
- Need to apply 3 patches, one of which isn't ready, or
- Revert the original patch.

Given those options, I think reverting is the best.




Some additional information:
This bug requires the tutorial to be dismissed with the "OK Got It" button on the Cast dialog in order to repro. It will then repro every time the dialog is displayed.

The M68 bug ( http://crbug.com/840036 ) requires the tutorial NOT be dismissed and repros about 1-in-10 attempts. You can use a different user data dir in order to effectively reset the tutorial: $ out/Release/Chromium.app/Contents/MacOs/Chromium --user-data-dir=/tmp/owrgoiwrg or whatever random subdir

This bug requires manual review: M67 has already been promoted to the beta branch, so this requires manual review
Please contact the milestone owner if you have questions.
Owners: cmasso@(Android), cmasso@(iOS), kbleicher@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Is this revert (https://chromium-review.googlesource.com/c/chromium/src/+/1013346 ) look good in canary and safe to merge to M67?
 

That revert CL has not yet been through a canary. FWIW, it takes away new code and restores old, well-tested code.

We don't want to apply the revert to M68 canary. M68 has many other patches that would need to be reverted in order to revert this. And in M68 things are mostly fixed.

IIUC, there are no more M67 canaries and the last M68 beta cuts tomorrow, right?

Is there any way we can test this on just a M67 branch? If not, I can whip up a list of all the patches which need to be reverted in M68 to test on the Canary there but we'll still miss the last M67 beta which seems more important.

M67 is already in beta and there are no more M67 canaries.
We only have 2 more beta releases left for M67. As revert can't be tested on M68 canary, approving merge for revert (https://chromium-review.googlesource.com/c/chromium/src/+/1013346) to M67 branch 3396 without canary coverage based on comment #20 and #23.


Also wanted check is this bug also 840036 repro on M67 and will be fixed by this revert merge? OR  Bug 840036  is specific to M68?

 Bug 840036  does not currently happen in M67. It only happens in M68.

If we begin applying the patches to fix the M67 bug (rather than this revert), we'll bring in the M68 bug as well. We don't yet have a fix for that bug.

It may be easy to find & fix, but we are low on time for M67. So I would prefer to fix it for M68.

Yeah, agree. We've to be extra careful with M67 as it is going to Stable soon. Pls merge the revert (https://chromium-review.googlesource.com/c/chromium/src/+/1013346) to M67 branch 3396. 

I'm making  Bug 840036  as M68 Stable blocker for tracking purpose. Pls feel free to modify the label if needed. Thank you again.

A CL with the revert applied to M67 is ready:
https://chromium-review.googlesource.com/c/chromium/src/+/1058124

Applying "Merge-merged-3396" label and removing "Merge-Approved-67" label per comment #27.

For testers:
Go to the overflow menu -> "Cast..."
This will bring up a window that should look normal.
It will animate a spinning circle.

The bug would show a non-animated dot instead of a spinning circle.

(Make sure to test with and without the tutorial. The tutorial is a bunch of text in the blue top of that Cast window. If you click the "OK, Got It" button the tutorial will go away. You can bring it back on Mac by launching Chrome with the command line parameter --user-data-dir=/tmp/kljhdfh or use whatever random subdirectory name.)

As per the original comment to verify this issue, Chromecast device is required. Due to unavailability of the device with our team we are unable to verify the fix and adding the label 'TE-Hardware-Dependency'.

Thanks!

Hi pnangunoori@chromium.org, this bug's verification steps don't need a Chromecast device. 

I was able to download and verify the latest M67 beta. This bug seems fixed. Thanks everybody!

Thank you  cblume@ for verifying on beta.