+olivierrobin to help triage.

Please assess severity

Michael used to work on context menu, assigning it to him

Just to clarify, regarding step 2 in c#0, in iOS Chrome terminology this is the iOS System Copy/Paste |UIMenuController|. (Not Chrome's Context Menu.)

I looked into this and cannot find the root cause even though I can reproduce the problem. (Text can be selected on PDF files, but there is no menu displayed above it.)

I can reproduce the issue in Chrome, but not in ios_web_view_shell. Neither removing the creation of ContextMenuController nor removing injected scripts fixed the issue. I also added breakpoints where UIMenuController is customized and in instances of |UIResponder canPerformAction:withSender:| and none of them hit so we are not directly preventing or removing items from the menu.

There may be some touch handling or a gesture recognizer which is preventing its display. Added eugenebut@ and marq@ in case they have any ideas where to look.

According to bugreport, this used to work in 68.0.3440.83. Can we bisect the culprit CL?

michaeldo - any update here? Stable cut is next week.

Lindsay, can someone from test team bisect this?

Tested on 68.0.3440.83 Beta (last Beta build) Good build
iPhone X iOS 11.4.1 
Issue is not reproducible.

Tested on 69.0.3497.4 Beta (first Beta build) Bad build
iPhone X iOS 11.4.1 
Issue is reproducing

Please let me know if there is any thing else I can help with.

Tested on 69.0.3446.0 Canary (last Beta build) Good build
iPhone X iOS 11.4.1 
Issue is not reproducible.
https://drive.google.com/file/d/12cez6WXX0veoa-oMyrHwg4GSlshjhb8x/view

Tested on 69.0.3447.0 Canary (first Beta build) Bad build
iPhone X iOS 11.4.1 
Issue is reproducing
https://drive.google.com/file/d/1r21vs2UnbDLIvHjMk5Jb7CjZubaZgAhP/view

justincohen@, it looks like your change to GetFirstResponder in crrev.com/c/1050192 broke this menu on PDFs.

Any ideas why this could be?

No, and I assume we don't want to revert this change as it fixes a crash deep inside UIKit. 

Does GetFirstResponder() not work on iOS 11 for pdfs?

It looks like the first responder is not a UIView, but a UIResponder subclass, UIPDFViewTouchHandler.h

Instead of returning nil in the new implementation, should we use the old implementation in that case? I don't know enough about the context of the crash if this would be ok. But IIUC, it is certainly not required that a UIView be the first responder. (So another bug similar to this could exist elsewhere.)

Also, on iOS 12, the first responder is the WKWebView which is why the new method works and the problem is only seen on iOS 11.

shbarezer@, can you confirm the behavior on iOS 11.4.1 in M68?

If I perform the fix as I suggest in c#13, I see the menu. However, I only see the "Select All" item. The "Copy" item is missing, which makes the menu not useful.

Tested on 68.0.3440.83 beta on iPhone X iOS 11.4.1, 
Behavior is fine - context menu has "Copy/Select All"


https://drive.google.com/file/d/1OLltSdt2xBaSxd0jDfQDO1eGLYBU1SG_/view

Any update here? Stable cut is this week.

Just saw the discussion on dynamite. Since this is currently broken in M69 stable and we're this close to M70 stable, I'm punting this to M71 stable. But this should really be fixed in M71.

We need to determine what to do here. Adding more people and summarizing the chat discussion below.

In summary, this missing menu on iOS 10/11 is due to a crash fix: crrev.com/c/1050192. That crash was caused by our old method of getting the first responder. The fix was to instead loop through the views to find the first responder. This doesn't work if the first responder is not a view. (This can be the case when the first responder is a UIResponder subclass, such as is the case when WKWebView displays a pdf on iOS 10/11.)

Our options are:
1) Use our old method of getting the first responder only if it isn't in the view hierarchy. This will likely bring back some of the crashes, but it will also fix this menu because it will use the old code.

2) Keep the crash as always fixed, but mark this as WontFix. This means users can not copy text from a PDF unless they have iOS 12. The side effect here is that in the future, there may be other cases where the first responder is not a view, and we will fail to discover it there as well.

3) Stop calling |becomeFirstResponder| on the WKWebView in BVC. Currently, the BVC makes the WKWebView the first responder if there isn't one. This looks like it is to improve support for keyboard commands as they will not work if there isn't a first responder.

I think our best option is to go with #1 even though it may introduce some crashes (unless we are ok with #2 and marking this as WontFix).

I think we should not go with #2 especially given that M-71 might be our last iOS 10 release. Option #3 can break keyboard support, which is pretty bad too.

Can we use option #1 only for iOS 11 and 10 to prevent unnecessary crashes in iOS 12?

In this case, the crashy code would never hit on iOS 12, but I don't recommend explicitly checking for OS version just in case there is another part of the system that is first responder, but not in the view hierarchy. (We could add a UMA event though to see if it's ever hit on iOS 12?)

I am just a bit concerned with 1 because in https://bugs.chromium.org/p/chromium/issues/detail?id=816427&desc=2#c2
the reproduction steps involved opening a PDF.

As we have reproduction steps, do we know the conditions in which it happens?
What is this accessibilityQuickSpeakLocalizedString?

See https://bugreport.apple.com/web/?problemID=38752582, which is closed as a dup of 32644609, which is still marked Open.

(I can't see that radar, can you check the account it was filed with?)

Also, it looks like the new implementation might not actually fix the crash. PTAL at https://crash.corp.google.com/browse?q=product_name%3D%27Chrome_iOS%27+AND+STRPOS%28expanded_custom_data.ChromeCrashProto.magic_signature_1.name%2C+%27GetFirstResponder%27%29+%3E+0

If the new GetFirstResponder implementation is also crashing, should we just revert it?

Option #1 sounds good to me (ref. Comment 19)

michaeldo@ are you saying you do not see 38752582 on https://bugreport.apple.com?  

Your crash/ link shows a number of very old crashes.  The only crashes I see with the new implementation are jailbroken crashes (there are 13).  Are you seeing something else?

Just to note, the previous crasher resulted in over 700 crashes a day.  Please don't revert this for 13 jailbroken crashes.

I'm fine with option 1 as long as it doesn't reintroduce a large number of crashes per day.

Is the initial bug fixed on iOS12?

To be clear, Option 1 seems to be the only sensible option. Am I missing something?

I believe per 839842 and 816427 this never happened on iOS12.

srikanthg@ do you recall?

iOS12 was not available at the time of fixing Issue 816427. So no data from crashes.
I installed the M64 build where we had this originally on iOS12 devices and the issue is not reproducible. So I believe that the bug was fixed.

Re #c26, correct. I can't see the bugreport and going to the link directly redirects to  https://bugreport.apple.com/web/?bugaccess (I verified the account is correct, maybe there is an issue with the site.) What is the title of the bug? maybe searching that way would work?

I was looking at the crashes from 816427, it looks like 839842 is the bulk of the crashes. There are lots of reports there and that's correct (per #c27) that I don't see any on iOS 12.
https://crash.corp.google.com/browse?q=product_name%3D%27Chrome_iOS%27+AND+expanded_custom_data.ChromeCrashProto.magic_signature_1.name%3D%27_platform_memmove%27

Re #c28, I thought number 1 was also the sensible thing, but there is concern about re-introducing the crashes on crrev.com/c/1261962. I think that since the potential crashes would only be a subset, it would be ok since we can fix this functionality. If we don't want to risk those crashes, we can decide to live with this bug (and potential others in the future where we incorrectly take the first responder away from a UIResponder subclass).

#31 title is "[MobileSafari] Crash in QuickSpeak in 11.3b6"

If the bug is not happening on iOS12, I would vote for solution 1bis

- Do an experiment on iOS12 with the old method which is faster and handles more cases (it is possible that we miss other UIResponder, now or in future iOS versions). If the experiment does not show any crash, revert the fix for iOS12+

- for iOS11-, do option #1 

I agree on iOS12 we should go back to using the sendEvent hack.

PTAL at itemized options here (sorry, internal doc): https://docs.google.com/document/d/1t3Tt6ca-RJZGFO1gDAf6MW85OhqTHtYwfkhUc-qVnno/edit?usp=sharing

Yes, I know the title of the bug specifies 11.3b6, it is more widespread than that. I believe we fixed the bug before iOS 12 was released, so yes, we can't revert without doing an experiment to see if there are actually iOS 12 crashes or not.

justincohen@, there is a flag already in place, were any experiments already done or was it just added in case the implementation needed to be reverted after shipping?

The later, there are no preexisting experiments.

rdar://45235249 filed - "[Chrome] UIKit does not provide access to firstResponder."

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

commit 9cdf6a5127883a95220d7b3c9da0cb8b8b251871
Author: Mike Dougherty <michaeldo@chromium.org>
Date: Thu Oct 18 00:44:57 2018

Do not use GetFirstResponderSubview on iOS 10.

When a WKWebView is displaying a PDF, the first responder is not in the view hierarchy (but a class adhering to UIResponder).

Bug:  882470 
Cq-Include-Trybots: luci.chromium.try:ios-simulator-cronet;luci.chromium.try:ios-simulator-full-configs
Change-Id: I6f9e993e740541b7cf009b1d11e16183b409fdae
Reviewed-on: https://chromium-review.googlesource.com/c/1284438
Commit-Queue: Mike Dougherty <michaeldo@chromium.org>
Reviewed-by: Mark Cogan <marq@chromium.org>
Cr-Commit-Position: refs/heads/master@{#600620}
[modify] https://crrev.com/9cdf6a5127883a95220d7b3c9da0cb8b8b251871/ios/chrome/browser/autofill/form_suggestion_controller_unittest.mm
[modify] https://crrev.com/9cdf6a5127883a95220d7b3c9da0cb8b8b251871/ios/chrome/browser/ui/uikit_ui_util.mm

I am working on landing an experiment to see if this crash happens on iOS 12.

Requesting merge of crrev.com/c/1284438 into M71. This CL forces the old GetFirstResponder implementation to be used on iOS 10 so that those users are in a known good state.

Your change meets the bar and is auto-approved for M71. Please go ahead and merge the CL to branch 3578 manually. Please contact milestone owner if you have questions.
Owners: benmason@(Android), kariahda@(iOS), kbleicher@(ChromeOS), govind@(Desktop)

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

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

commit 7cc4dc6f4ae9adddd1e271599ed8455c876967e8
Author: Mike Dougherty <michaeldo@chromium.org>
Date: Tue Oct 23 05:27:06 2018

Do not use GetFirstResponderSubview on iOS 10.

When a WKWebView is displaying a PDF, the first responder is not in the view hierarchy (but a class adhering to UIResponder).

Bug:  882470 
Cq-Include-Trybots: luci.chromium.try:ios-simulator-cronet;luci.chromium.try:ios-simulator-full-configs
Change-Id: I6f9e993e740541b7cf009b1d11e16183b409fdae
Reviewed-on: https://chromium-review.googlesource.com/c/1284438
Commit-Queue: Mike Dougherty <michaeldo@chromium.org>
Reviewed-by: Mark Cogan <marq@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#600620}(cherry picked from commit 9cdf6a5127883a95220d7b3c9da0cb8b8b251871)
Reviewed-on: https://chromium-review.googlesource.com/c/1296054
Reviewed-by: Mike Dougherty <michaeldo@chromium.org>
Cr-Commit-Position: refs/branch-heads/3578@{#257}
Cr-Branched-From: 4226ddf99103e493d7afb23a4c7902ee496108b6-refs/heads/master@{#599034}
[modify] https://crrev.com/7cc4dc6f4ae9adddd1e271599ed8455c876967e8/ios/chrome/browser/autofill/form_suggestion_controller_unittest.mm
[modify] https://crrev.com/7cc4dc6f4ae9adddd1e271599ed8455c876967e8/ios/chrome/browser/ui/uikit_ui_util.mm

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

Commit: 7cc4dc6f4ae9adddd1e271599ed8455c876967e8
Author: michaeldo@chromium.org
Commiter: michaeldo@chromium.org
Date: 2018-10-23 05:27:06 +0000 UTC

Do not use GetFirstResponderSubview on iOS 10.

When a WKWebView is displaying a PDF, the first responder is not in the view hierarchy (but a class adhering to UIResponder).

Bug:  882470 
Cq-Include-Trybots: luci.chromium.try:ios-simulator-cronet;luci.chromium.try:ios-simulator-full-configs
Change-Id: I6f9e993e740541b7cf009b1d11e16183b409fdae
Reviewed-on: https://chromium-review.googlesource.com/c/1284438
Commit-Queue: Mike Dougherty <michaeldo@chromium.org>
Reviewed-by: Mark Cogan <marq@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#600620}(cherry picked from commit 9cdf6a5127883a95220d7b3c9da0cb8b8b251871)
Reviewed-on: https://chromium-review.googlesource.com/c/1296054
Reviewed-by: Mike Dougherty <michaeldo@chromium.org>
Cr-Commit-Position: refs/branch-heads/3578@{#257}
Cr-Branched-From: 4226ddf99103e493d7afb23a4c7902ee496108b6-refs/heads/master@{#599034}

The NextAction date has arrived: 2018-10-26

@michaeldo is this completely merged now? Is this considered fixed and ready for verification?

Yes, it is ready for verification. In summary: The PDF selection menu has been fixed on iOS 10 and also works on iOS 12. It remains broken on iOS 11, which is expected.

Verified in 

Build: 71.0.3578.28 Beta 
OS versions: 11.4.1, 12.0.1, 12.1 Beta, 10.3.3
Devices: iPhone X, iPhone 8, iPhone 7plus, iPad Air, iPad Pro

Pdf text selection menu copy and select all is displayed for iOS 10 and iOS 12. 

As mentioned in c#48 Menu is not displayed in iOS 11

Link to video:

iOS 11: https://drive.google.com/file/d/1zjRypqQj1Z4ofCB8lwf2NB2N3NSBlEDm/view?usp=sharing

iOS 12: https://drive.google.com/file/d/1YZ6fS6y9GYVrzG9_YLqguHz5HYs-NZ1m/view?usp=sharing