1373220 - Crash in nsCOMPtr_base::assign_with_AddRef | nsBaseWidget::AddChild

Status: VERIFIED FIXED

(Core :: Widget: Win32, defect, P2)

Description

[[:philipp]]

This bug was filed from the Socorro interface and is 
report bp-f164bedd-f815-46ff-b462-e930a0170615.
=============================================================
Crashing Thread (0)
Frame 	Module 	Signature 	Source
0 	xul.dll 	nsCOMPtr_base::assign_with_AddRef(nsISupports*) 	xpcom/base/nsCOMPtr.cpp:44
1 	xul.dll 	nsBaseWidget::AddChild(nsIWidget*) 	widget/nsBaseWidget.cpp:580
2 	xul.dll 	nsWindow::SetParent(nsIWidget*) 	widget/windows/nsWindow.cpp:1261
3 	xul.dll 	nsViewManager::ReparentChildWidgets(nsView*, nsIWidget*) 	view/nsViewManager.cpp:829
4 	xul.dll 	nsViewManager::ReparentChildWidgets(nsView*, nsIWidget*) 	view/nsViewManager.cpp:842
5 	xul.dll 	nsViewManager::ReparentWidgets(nsView*, nsView*) 	view/nsViewManager.cpp:863
6 	xul.dll 	nsViewManager::InsertChild(nsView*, nsView*, nsView*, bool) 	view/nsViewManager.cpp:920
7 	xul.dll 	InsertViewsInReverseOrder 	layout/generic/nsSubDocumentFrame.cpp:1100
8 	xul.dll 	nsSubDocumentFrame::BeginSwapDocShells(nsIFrame*) 	layout/generic/nsSubDocumentFrame.cpp:1125
9 	xul.dll 	nsFrameLoader::SwapWithOtherLoader(nsFrameLoader*, nsIFrameLoaderOwner*, nsIFrameLoaderOwner*) 	dom/base/nsFrameLoader.cpp:1845
10 	xul.dll 	nsXULElement::SwapFrameLoaders(nsIFrameLoaderOwner*, mozilla::ErrorResult&) 	dom/xul/nsXULElement.cpp:1627
11 	xul.dll 	nsXULElement::SwapFrameLoaders(nsXULElement&, mozilla::ErrorResult&) 	dom/xul/nsXULElement.cpp:1607
12 	xul.dll 	mozilla::dom::XULElementBinding::swapFrameLoaders 	obj-firefox/dom/bindings/XULElementBinding.cpp:8090
13 	xul.dll 	mozilla::dom::GenericBindingMethod(JSContext*, unsigned int, JS::Value*) 	dom/bindings/BindingUtils.cpp:2958
...

a number of user comments in those crash reports say they where trying to close a firebug window, like in
bp-2c91fdf3-e247-4c89-abfd-a8f9f0170520, bp-ed8efa01-9678-445d-9161-f61370170521 & bp-f164bedd-f815-46ff-b462-e930a0170615.
i wasn't able to reproduce this myself when playing around a little bit with the addon.

crashes with this signature when firebug is present (for others, see bug 1367686) seem to have started sometime in the 55.0a1 nightly cycle, the first instance was in build 20170514030207. this is the search i'm looking at for that:
https://crash-stats.mozilla.com/search/?signature=~nsBaseWidget%3A%3AAddChild&addons=~firebug%40software.joehewitt.com&release_channel=nightly&product=Firefox&date=%3E%3D2017-03-15T12%3A16%3A01.000Z&_facets=signature&_facets=version&_facets=user_comments&_facets=build_id&_facets=install_time&_facets=platform_pretty_version&_facets=useragent_locale&_facets=cpu_arch#facet-build_id

a portion of them is crashing in an address indicating a UAF situation.

Comment 1

[Ryan VanderMeulen [:RyanVM]]

[Tracking Requested - why for this release]: Topcrash in early Beta 55 builds

Comment 2

[Ryan VanderMeulen [:RyanVM]]

Jim, are you the right person to start the investigation here since it's in Widget code?

Comment 3

[Andrew McCreight [:mccr8]]

Looks like it requires user interaction and involves chrome stuff.

Comment 4

[Jim Mathies [:jimm]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #1)
> [Tracking Requested - why for this release]: Topcrash in early Beta 55 builds

This doesn't look like a 55 top crasher to me. It's a pretty bad crash though, showing up in 53 currently. Doesn't appear in 54 yet.

Definitely need to fix this.

Comment 5

[Ryan VanderMeulen [:RyanVM]]

Keep in mind that 55 is still being heavily throttled, so the absolute number of crashes may be a misleading indication of the severity :)

Comment 6

[Mike Taylor [:miketaylr]]

Jim, can you help prioritize this?

Comment 7

[Daniel Veditz [:dveditz]]

The Firebug correlation is a red herring. Only a third of the crashes with this signature have firebug installed, and those are largely beta users (as you might expect). Since processing release crashes is throttled to 10% the actual fraction of crashing users with firebug installed is even lower.

A bunch of the UAF stacks I looked at were under nsPluginInstanceOwner::CreateWidget() (about a quarter of them). Another third were under nsViewManager::InsertChild.

I don't know that this always requires user interaction. A lot of the ones with comments mention that (e.g. comment 0, or moving their twitch.tv streams to another window), but a bunch don't. sec-moderate might be too conservative.

Comment 8

[Mike Taylor [:miketaylr]]

(In reply to Daniel Veditz [:dveditz] from comment #7)
> The Firebug correlation is a red herring. Only a third of the crashes with
> this signature have firebug installed, and those are largely beta users (as
> you might expect). Since processing release crashes is throttled to 10% the
> actual fraction of crashing users with firebug installed is even lower.
> 
> A bunch of the UAF stacks I looked at were under
> nsPluginInstanceOwner::CreateWidget() (about a quarter of them). Another
> third were under nsViewManager::InsertChild.
> 
> I don't know that this always requires user interaction. A lot of the ones
> with comments mention that (e.g. comment 0, or moving their twitch.tv
> streams to another window), but a bunch don't. sec-moderate might be too
> conservative.

Maybe we should reconsider this being a P2 given this comment.

Comment 9

[Mike Taylor [:miketaylr]]

Andrew, what do you think (re: comment 8)?

Comment 10

[Andrew McCreight [:mccr8]]

(In reply to Mike Taylor [:miketaylr] (55 Regression Engineering Owner) from comment #9)
> Andrew, what do you think (re: comment 8)?

I don't have any opinion on priority flags.

Comment 11

[[:philipp]]

i found a way to reproduce the crash with firebug and when regression-testing the issue it appears that bug 1358832 introduced the crash with the addon.

Comment 12

[[:philipp]]

str in a new profile: (e10s has to be off)
- install firebug
- go to a website
- click on the firebug icon
- click on the button to detach the firebug panel to a new window
- click on the down arrow next to script to enable that panel
- click on the reload button on the website
- click on the script tab in the firebug window to focus it again
- go to the windows task bar and close the firebug window
- firefox crashes

Comment 13

[[:philipp]]

Attachment: str.mp4

Comment 14

[Olli Pettay [:smaug]]

Are there any UAF crashes? Based on comments yes, but I saw only null+offset crashes with the stack traces similar to comment 0.

Comment 15

[[:philipp]]

see https://crash-stats.mozilla.com/report/index/e36118a7-0621-4099-8524-718180170705 for an example of UAF

Comment 16

[Olli Pettay [:smaug]]

Hmm, I can't seem to play that mp4 on linux and I don't understand comment 12.
"click on the down arrow next to script to enable that panel" ?
I don't have "script" tab in firebug window.

Comment 17

[[:philipp]]

the STR only work when e10s is switched off (browser.tabs.remote.autostart.2 = false), otherwise firebug is just a skin for the built-in devtools). the video should work with alternative players like vlc...

Comment 18

[Olli Pettay [:smaug]]

oh, e10s off.

By any chance, could someone who can reproduce this try to make the following variables strong
http://searchfox.org/mozilla-central/rev/5dadcbe55b4ddd1e448c06c77390ff6483aa009b/view/nsViewManager.cpp#823-824
So, use nsCOMPtr<nsIWidget> there, not nsIWidget*

Comment 19

[Olli Pettay [:smaug]]

but let me retry

Comment 20

[Olli Pettay [:smaug]]

Still no luck.

Comment 22

[Jim Mathies [:jimm]]

David, could you please take a look at this and the data in dupe bug 1367686 and see if you can track down a fix.

Comment 23

[David Parks (dparks) (he/him) [:handyman]]

Attachment: widgetParent.patch

Patch is pretty straightforward.  GetParent() is giving the new parent when we want the old one.

-----

The STR in comment 12 is very finicky (kudos to the OP for finding it).  Slight deviations negate it and there are a lot of steps.  The main things that tripped me up were:

* You only restart the browser to turn off e10s.  Specifically, don't restart after installing Firebug.
* There currently is no "down arrow next to [the script tab]" but you just need to select the tab.  (An arrow then appears... you dont need to push that)
* After reloading the page, the script tab will still be selected.  The idea there is just to refocus the window.

Tests:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=90b593e0d1ae02250c55c53297db0ea085f54165

Comment 24

[Jim Mathies [:jimm]]

Looks like this was introduced in bug 530070.

Comment 25

[David Parks (dparks) (he/him) [:handyman]]

sec-moderate... so it looks like we are good to go.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6cb0fcbcdd504201af4caa94ad9a7d8383b43926

I've confirmed that this grafts cleanly to Beta and ESR52. Please request approval on the patch when you're comfortable doing so.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6cb0fcbcdd50

Comment 28

[David Parks (dparks) (he/him) [:handyman]]

Comment on attachment 8885994 [details] [diff] [review]
widgetParent.patch

Here we go --

(ESR52)
[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
Fairly high volume Windows crash
User impact if declined: 
Crashes
Fix Landed on Version:
56
Risk to taking this patch (and alternatives if risky): 
Low.  This couldn't introduce an issue worse than the UAF it fixes.
String or UUID changes made by this patch: 
None

--------

(Beta)
Approval Request Comment
[Feature/Bug causing the regression]:
Windows widget, specifically window parentage
[User impact if declined]:
Crashes from normal usage
[Is this code covered by automated tests?]:
No
[Has the fix been verified in Nightly?]:
Yes
[Needs manual test from QE? If yes, steps to reproduce]: 
I think yes.  STR is in comment 12, with some notes on STR gotchas in comment 23.
[List of other uplifts needed for the feature/fix]:
None
[Is the change risky?]:
No
[Why is the change risky/not risky?]:
This couldn't introduce an issue worse than the UAF it fixes.
[String changes made/needed]:
None

Comment 29

[Daniel Veditz [:dveditz]]

(In reply to Jim Mathies [:jimm] from comment #24)
> Looks like this was introduced in bug 530070.

Wow, much older than I would have thought given the crash volume spike when 53 was released. Maybe something else changed and happened to start using the buggy function in just the right way?

Comment 30

[Julien Cristau [:jcristau]]

Comment on attachment 8885994 [details] [diff] [review]
widgetParent.patch

uaf in windows widget code, beta55+

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/74382cf3e203

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/5dd82ab194b9

Comment 33

[Andrei Vaida [:avaida]]

Reproduced the crash on an affected build (55.0b10, 20170717063821, e26b1f5d635e) [1] using the str from Comment 12 on Windows 10 x64.

I can confirm these str are no longer crashing on Windows 10 x64 and Windows 7 x64 using:
    - 52.2esr (20170723091719, 3856be3d222f)
    - 55.0b11 (20170720101430, e5f14b9ae6c4)
    - 56.0a1 (2017-07-23, 20170723030206)    

[1] bp-ef4173ac-4784-4d79-97b7-6aea30170724