1227461 - Firefox crashes when ctrl+shift+tab in "Page Info"

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[[e-mail]]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:42.0) Gecko/20100101 Firefox/42.0
Build ID: 20151029151421

Steps to reproduce:

view "Page Info"
Hit Ctrl + Shift + Tab


Actual results:

Firefox freezes.

Comment 1

[Loic]

WFM with FF42 on Win 7.

Could you test:
1) in safe mode:
https://support.mozilla.org/en-US/kb/troubleshoot-firefox-issues-using-safe-mode

2) with a fresh profile:
https://support.mozilla.org/en-US/kb/profile-manager-create-and-remove-firefox-profiles

Is it better?

Comment 2

[[e-mail]]

safe mode,fresh profile,fresh profile in safe mode
FF crashed in all conditions.

Comment 3

[Loic]

If FF crashed, could you provide crash IDs (bp-...) from about:crashes, please.

Comment 4

[[e-mail]]

Recent report does not exist.
I have a question
crash isn't freeze?

Comment 5

[Loic]

Nope.

Comment 6

[[e-mail]]

Retried several times.

bp-def64082-727c-43f9-9c0a-0552e2151124

Is this ok?

Comment 7

[Alice0775 White]

Regression window:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=3054f69fb5a1&tochange=dc9d58b43abf

Suspect: Bug 1132518

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

hmm, we have endless loop in nsFocusManager::GetNextTabbableContent

Comment 9

[Neil Deakin]

Attachment: Fixes infinite loop

This fixes the infinite loop. The issue is that when tabbing forwards we can easily detect when we hit the end of the document tree and focus the first chrome element. When going backwards though, we want to continue iterating from the end again. In the page info dialog though, there aren't any other documents, so we need to stop iterating once we hit the original starting content.

This reveals some incorrect or inconsistent behaviour. When performing a document navigation when there aren't any other documents, we currently focus the first element when going forward but leave the focus as is when going backwards. We should do one or the other in both cases. Pre-bug 1132518, we focused the first element in both cases.

I want to look at this some more.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Attachment: wip

I had just written this before you uploaded your patch, Neil :)

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8691521 [details] [diff] [review]
Fixes infinite loop

This doesn't seem to work if you first tab couple of times in Page Info and then press shift+F6. The loop is there again.

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8691638 [details] [diff] [review]
wip

Apparently I attached wrong patch

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Attachment: wip

So this patch aims to ensure
if (forDocumentNavigation && (forward || mayFocusRoot || popupFrame)) {
is true at some point even when going backwards.

Child process case is already taken care of in
docShell->TabToTreeOwner(forward, forDocumentNavigation, &tookFocus); call.

Comment 14

[Neil Deakin]

Comment on attachment 8691647 [details] [diff] [review]
wip

This is good fix for the simple case. It still hangs if there's a child iframe with nothing focusable in it.

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

Attachment: brute force approach

There seems to be couple of different ways to end up endless loop here
(all the ignoreTabIndex and doNavigation handling etc is rather fragile), so
I ended up using brute force approach here to be safe:
when we know the next run would just do that same as the current one, bail out. The code didn't become too horrible, IMO :)

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8692698 [details] [diff] [review]
brute force approach

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e34b52a099a7

Comment 17

[Neil Deakin]

Attachment: docnavnoinfinite

Wouldn't the following brute-force method be simpler?

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8694374 [details] [diff] [review]
docnavnoinfinite

Yeah, I think this is fine too.

Comment 19

[Neil Deakin]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c6bacc29209b5a8afb28b296b1647ab461478ba6
Bug 1227461, don't iterate multiple times past the top-level document during tab navigation, prevents hang navigating in page info dialog, r=smaug

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c6bacc29209b

Comment 22

[Cornel Ionce [:noni] [Hubs QA]]

Reproduced the initial issue on Windows 10 x64 using Firefox 44.0.2, build ID: 20160210153822.

Confirming the fix across platform on Firefox 45 beta 6, build ID 20160215141016