1528034 - Browser tab crashes when tabbing after the previous tab target is removed

Status: RESOLVED FIXED

(Core :: DOM: UI Events & Focus Handling, defect, P3)

Description

[Mark Striemer [:mstriemer]]

I originally noticed this while working on about:addons, where the entire browser would crash when this happened. I copied my code to glitch.com [1] to check if it happens in web content and it will crash [2] the tab there.

STR

1. Tab to focus the Disable option
2. Press space/enter to hide the element
3. Press Tab

Expected results: The next tab target is selected
Actual results: The tab crashes

[1] https://shrouded-saturday.glitch.me/
[2] https://crash-stats.mozilla.com/report/index/3250602e-9564-4373-8a10-a65290190214

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Looks like that nullptr access in nsFocusManager.

Comment 2

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

I was looking at the crash report and it wasn't quite clear to me where the null pointer crash actually happens.

Comment 3

[Neil Deakin]

The crash seems to happen at https://searchfox.org/mozilla-central/rev/e6520f0a4dd5d7474c32a1164744953ea59be0d0/dom/base/nsFocusManager.cpp#3162 as the result of the call to FindOwner -- which can return null -- is then passed to IsHostOrSlot which expects its argument to be non-null.

I don't know much about any of this code since it was added for some shadow dom stuff.

Comment 4

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

Indeed, thanks.

Comment 5

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

Attachment: null check

we can just make IsHostOrSlot null-safe

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dc08d3602655
make IsHostOrSlot null-safe, r=enn

Comment 7

[Daniel Varga [:dvarga]]

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

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Should we take this on Beta too?

Comment 9

[Emilio Cobos Álvarez [:emilio]]

Bug 1513141 only refactored code, there was no null-check before either.

Comment 10

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

Comment on attachment 9044461 [details] [diff] [review]
null check

Beta/Release Uplift Approval Request

• Feature/Bug causing the regression: Bug 1471947
• User impact if declined: Crashes.
  Possibly a regression from enabling shadow DOM.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Null check
• String changes made/needed: NA

Comment 11

[Liz Henry (:lizzard)]

Comment on attachment 9044461 [details] [diff] [review]
null check

Fix for a rare crash. The fix is simple so let's bring it to beta 12.

Comment 12

[Razvan Maries]

https://hg.mozilla.org/releases/mozilla-beta/rev/20cd9dd27bfc