Closed Bug 1227461 Opened 4 years ago Closed 4 years ago

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

Categories

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

42 Branch
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla45
Tracking Status
firefox42 --- affected
firefox43 --- affected
firefox44 --- affected
firefox45 --- verified
firefox-esr38 --- unaffected

People

(Reporter: mrgh1bugzillamozilla, Assigned: enndeakin)

References

Details

(Keywords: hang, regression)

Attachments

(1 file, 4 obsolete files)

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.
Component: Untriaged → Page Info Window
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?
Flags: needinfo?(mrgh1bugzillamozilla)
safe mode,fresh profile,fresh profile in safe mode
FF crashed in all conditions.
Flags: needinfo?(mrgh1bugzillamozilla)
If FF crashed, could you provide crash IDs (bp-...) from about:crashes, please.
Recent report does not exist.
I have a question
crash isn't freeze?
Nope.
Retried several times.

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

Is this ok?
Keywords: hang
Status: UNCONFIRMED → NEW
Ever confirmed: true
Regression window:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=3054f69fb5a1&tochange=dc9d58b43abf

Suspect: Bug 1132518
Blocks: 1132518
Severity: normal → critical
Flags: needinfo?(enndeakin)
hmm, we have endless loop in nsFocusManager::GetNextTabbableContent
Attached patch Fixes infinite loop (obsolete) — Splinter Review
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.
Assignee: nobody → enndeakin
Status: NEW → ASSIGNED
Flags: needinfo?(enndeakin)
Attached patch wip (obsolete) — Splinter Review
I had just written this before you uploaded your patch, Neil :)
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 on attachment 8691638 [details] [diff] [review]
wip

Apparently I attached wrong patch
Attachment #8691638 - Attachment is obsolete: true
Attached patch wip (obsolete) — Splinter Review
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.
Attachment #8691647 - Flags: feedback?(enndeakin)
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.
Attachment #8691647 - Flags: feedback?(enndeakin) → feedback-
Attached patch brute force approach (obsolete) — Splinter Review
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 :)
Assignee: enndeakin → bugs
Attachment #8691521 - Attachment is obsolete: true
Attachment #8691647 - Attachment is obsolete: true
Attachment #8692698 - Flags: review?(enndeakin)
Attached patch docnavnoinfiniteSplinter Review
Wouldn't the following brute-force method be simpler?
Attachment #8694374 - Flags: feedback?(bugs)
Comment on attachment 8694374 [details] [diff] [review]
docnavnoinfinite

Yeah, I think this is fine too.
Attachment #8694374 - Flags: feedback?(bugs) → review+
Attachment #8692698 - Flags: review?(enndeakin)
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
https://hg.mozilla.org/mozilla-central/rev/c6bacc29209b
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 45
Assignee: bugs → enndeakin
Component: Page Info Window → DOM
Product: Firefox → Core
Target Milestone: Firefox 45 → ---
Attachment #8692698 - Attachment is obsolete: true
Flags: qe-verify+
Target Milestone: --- → mozilla45
See Also: → 1248404
See Also: 1248404
Duplicate of this bug: 1248404
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
Status: RESOLVED → VERIFIED
Flags: qe-verify+
QA Contact: cornel.ionce
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.