Closed
Bug 1442010
Opened 6 years ago
Closed 6 years ago
Crash in nsMenuPopupFrame::ShouldFollowAnchor
Categories
(Toolkit :: UI Widgets, defect, P1)
Tracking
()
RESOLVED
FIXED
mozilla63
People
(Reporter: philipp, Assigned: enndeakin)
Details
(Keywords: crash, csectype-uaf, sec-high, Whiteboard: [post-critsmash-triage][adv-main63+][adv-esr60.3+])
Crash Data
Attachments
(2 files)
881 bytes,
patch
|
tnikkel
:
review+
abillings
:
sec-approval+
|
Details | Diff | Splinter Review |
881 bytes,
patch
|
RyanVM
:
approval-mozilla-esr60+
|
Details | Diff | Splinter Review |
This bug was filed from the Socorro interface and is report bp-5231f334-613d-4a79-8695-930a60180224. ============================================================= Top 10 frames of crashing thread: 0 xul.dll nsMenuPopupFrame::ShouldFollowAnchor layout/xul/nsMenuPopupFrame.cpp:2537 1 xul.dll nsMenuPopupFrame::ShouldFollowAnchor layout/xul/nsMenuPopupFrame.cpp:2560 2 xul.dll nsMenuChainItem::UpdateFollowAnchor layout/xul/nsXULPopupManager.cpp:120 3 xul.dll nsXULPopupManager::ShowPopupCallback layout/xul/nsXULPopupManager.cpp:975 4 xul.dll nsXULPopupManager::FirePopupShowingEvent layout/xul/nsXULPopupManager.cpp:1547 5 xul.dll nsXULPopupManager::ShowPopupAtScreen layout/xul/nsXULPopupManager.cpp:816 6 xul.dll nsXULPopupListener::LaunchPopup dom/xul/nsXULPopupListener.cpp:443 7 xul.dll nsXULPopupListener::HandleEvent dom/xul/nsXULPopupListener.cpp:219 8 xul.dll mozilla::EventListenerManager::HandleEventSubType dom/events/EventListenerManager.cpp:1118 9 xul.dll mozilla::EventListenerManager::HandleEventInternal dom/events/EventListenerManager.cpp:1293 ============================================================= these crashes on windows are apparently rising in volume with firefox 58 somewhat. windows 8.1 looks like it's overrepresented in crash stats. since there are reports from firefox 53 but none from 52esr, perhaps the crash is related to bug 1109868?
Comment 1•6 years ago
|
||
Enn/:tn, any idea what's going on here? Is it possible the frame is gone or something?
Flags: needinfo?(tnikkel)
Flags: needinfo?(enndeakin)
Assignee | ||
Comment 2•6 years ago
|
||
Tracing though, the only thing I can see is that a call to nsContentUtils::NotifyInstalledMenuKeyboardListener can happen (from SetCaptureState) and that does a whole bunch of ime stuff that perhaps could destroy the frame, although I don't know for sure. A possible quick fix is to just add another weakframe check just before the call to UpdateFollowAnchor at: https://hg.mozilla.org/releases/mozilla-release/annotate/849c090094db/layout/xul/nsXULPopupManager.cpp#l975 That said, if that is the issue, it may have caused different crashes before 1109868 as well as the frame is accessed immediately afterwards again.
Flags: needinfo?(enndeakin)
Updated•6 years ago
|
Comment 3•6 years ago
|
||
(In reply to Neil Deakin from comment #2) > A possible quick fix is to just add another weakframe check just before the > call to UpdateFollowAnchor at: > > https://hg.mozilla.org/releases/mozilla-release/annotate/849c090094db/layout/ > xul/nsXULPopupManager.cpp#l975 > > That said, if that is the issue, it may have caused different crashes before > 1109868 as well as the frame is accessed immediately afterwards again. I think that's worth trying out. WDYT?
Flags: needinfo?(enndeakin)
Updated•6 years ago
|
Flags: needinfo?(mconley)
Updated•6 years ago
|
Flags: needinfo?(mconley)
Assignee | ||
Comment 4•6 years ago
|
||
No harm in checking here.
Assignee: nobody → enndeakin
Status: NEW → ASSIGNED
Flags: needinfo?(enndeakin)
Attachment #9005155 -
Flags: review?(tnikkel)
Updated•6 years ago
|
Flags: needinfo?(tnikkel)
Attachment #9005155 -
Flags: review?(tnikkel) → review+
Comment 5•6 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/be6a30c44b959056e51e970ebe99a03829df32b5 https://hg.mozilla.org/mozilla-central/rev/be6a30c44b95
Group: toolkit-core-security → core-security-release
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox63:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Comment 6•6 years ago
|
||
This is rated sec-high and affects more than trunk, so it shouldn't have landed without sec-approval. https://wiki.mozilla.org/Security/Bug_Approval_Process Please request it retroactively so we can figure out what we need to do for other branches.
status-firefox61:
--- → wontfix
status-firefox62:
--- → wontfix
status-firefox-esr60:
--- → affected
tracking-firefox-esr60:
--- → ?
Flags: needinfo?(enndeakin)
Assignee | ||
Comment 7•6 years ago
|
||
Comment on attachment 9005155 [details] [diff] [review] Check if the frame has been destroyed before calling UpdateFollowAnchor [Security approval request comment] > How easily could an exploit be constructed based on the patch? Don't know what causes the crash. Happens in chrome-only code. Probably a tooltip or other chrome-specific popup UI getting deleted while it is being opened. > Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? This is a null-check. > Which older supported branches are affected by this flaw? > If not all supported branches, which bug introduced the flaw? Could theoretically happen since bug 1109868. > Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? No risk. > How likely is this patch to cause regressions; how much testing does it need? No risk.
Flags: needinfo?(enndeakin)
Attachment #9005155 -
Flags: sec-approval?
Comment 8•6 years ago
|
||
Well, I guess since we fixed it on trunk when it was 63, we just need to backport this to ESR60 for 60.3. Can we get a patch made and nominated for ESR60, please?
Flags: needinfo?(enndeakin)
Updated•6 years ago
|
Attachment #9005155 -
Flags: sec-approval? → sec-approval+
Assignee | ||
Comment 9•6 years ago
|
||
[Approval Request Comment] If this is not a sec:{high,crit} bug, please state case for ESR consideration: User impact if declined: crashes Fix Landed on Version: 63 Risk to taking this patch (and alternatives if risky): none String or UUID changes made by this patch: none See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.
Flags: needinfo?(enndeakin)
Attachment #9007230 -
Flags: approval-mozilla-esr60?
Updated•6 years ago
|
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Comment 10•6 years ago
|
||
Comment on attachment 9007230 [details] [diff] [review] Check if the frame has been destroyed before calling UpdateFollowAnchor, ESR60 Fixes a sec-high, approved for ESR 60.3.
Attachment #9007230 -
Flags: approval-mozilla-esr60? → approval-mozilla-esr60+
Comment 11•6 years ago
|
||
uplift |
https://hg.mozilla.org/releases/mozilla-esr60/rev/87be1b98ec9a
Updated•6 years ago
|
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main63+][adv-esr60.3+]
Updated•5 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•