nsNativeThemeWin seems to spin event loop during layout

VERIFIED FIXED in Firefox 24

Status

()

defect
--
critical
VERIFIED FIXED
6 years ago
5 years ago

People

(Reporter: smaug, Assigned: jimm)

Tracking

({assertion, crash, sec-moderate})

unspecified
mozilla25
x86_64
Windows 7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox21 wontfix, firefox22 wontfix, firefox23- wontfix, firefox24+ verified, firefox25+ verified, firefox-esr17- wontfix, b2g18 wontfix)

Details

(Whiteboard: [adv-main24+])

Attachments

(1 attachment)

This is what causes Bug 748156.

07:01:45     INFO -  ###!!! ASSERTION: This is unsafe! Fix the caller!: 'Error', file e:/builds/moz2_slave/m-cen-w32-d-000000000000000000/build/content/events/src/nsEventDispatcher.cpp, line 494
07:01:45     INFO -  nsEventDispatcher::DispatchDOMEvent(nsISupports *,nsEvent *,nsIDOMEvent *,nsPresContext *,nsEventStatus *) [content/events/src/nsEventDispatcher.cpp:692]
07:01:45     INFO -  nsGlobalWindow::DispatchEvent(nsIDOMEvent *,bool *) [dom/base/nsGlobalWindow.cpp:7928]
07:01:45     INFO -  nsGlobalWindow::DispatchEvent(nsIDOMEvent *,bool *) [dom/base/nsGlobalWindow.cpp:7909]
07:01:45     INFO -  nsContentUtils::DispatchEvent(nsIDocument *,nsISupports *,nsAString_internal const &,bool,bool,bool,bool *) [content/base/src/nsContentUtils.cpp:3532]
07:01:45     INFO -  nsContentUtils::DispatchTrustedEvent(nsIDocument *,nsISupports *,nsAString_internal const &,bool,bool,bool *) [content/base/src/nsContentUtils.cpp:3503]
07:01:45     INFO -  nsFocusManager::WindowLowered(nsIDOMWindow *) [dom/base/nsFocusManager.cpp:766]
07:01:45     INFO -  nsWebShellWindow::WindowDeactivated() [xpfe/appshell/src/nsWebShellWindow.cpp:418]
07:01:45     INFO -  nsWindow::DispatchFocusToTopLevelWindow(bool) [widget/windows/nsWindow.cpp:4080]
07:01:45     INFO -  nsWindow::ProcessMessage(unsigned int,unsigned int &,long &,long *) [widget/windows/nsWindow.cpp:5177]
07:01:45     INFO -  nsWindow::WindowProcInternal(HWND__ *,unsigned int,unsigned int,long) [widget/windows/nsWindow.cpp:4390]
07:01:45     INFO -  CallWindowProcCrashProtected [xpcom/base/nsCrashOnException.cpp:32]
07:01:45     INFO -  nsWindow::WindowProc(HWND__ *,unsigned int,unsigned int,long) [widget/windows/nsWindow.cpp:4342]
07:01:45     INFO -  USER32 + 0x186ef
07:01:45     INFO -  USER32 + 0x18876
07:01:45     INFO -  USER32 + 0x170f4
07:01:45     INFO -  USER32 + 0x1738f
07:01:45     INFO -  ntdll + 0x4642e
07:01:45     INFO -  USER32 + 0x17308
07:01:45     INFO -  USER32 + 0x1729f
07:01:45     INFO -  USER32 + 0xbf96
07:01:45     INFO -  USER32 + 0x1738f
07:01:45     INFO -  ntdll + 0x4642e
07:01:45     INFO -  QueryForButtonData [widget/windows/nsNativeThemeWin.cpp:119]
07:01:45     INFO -  nsIFrame::AddCSSMinSize(nsBoxLayoutState &,nsIFrame *,nsSize &,bool &,bool &) [layout/xul/base/src/nsBox.cpp:690]
07:01:45     INFO -  nsBoxFrame::GetMinSize(nsBoxLayoutState &) [layout/xul/base/src/nsBoxFrame.cpp:816]
07:01:45     INFO -  nsBoxFrame::GetPrefSize(nsBoxLayoutState &) [layout/xul/base/src/nsBoxFrame.cpp:768]
07:01:45     INFO -  nsSprocketLayout::GetPrefSize(nsIFrame *,nsBoxLayoutState &) [layout/xul/base/src/nsSprocketLayout.cpp:1320]
07:01:45     INFO -  nsBoxFrame::GetPrefSize(nsBoxLayoutState &) [layout/xul/base/src/nsBoxFrame.cpp:757]
07:01:45     INFO -  nsSprocketLayout::GetPrefSize(nsIFrame *,nsBoxLayoutState &) [layout/xul/base/src/nsSprocketLayout.cpp:1320]
07:01:45     INFO -  nsBoxFrame::GetPrefSize(nsBoxLayoutState &) [layout/xul/base/src/nsBoxFrame.cpp:757]
07:01:45     INFO -  nsSprocketLayout::GetPrefSize(nsIFrame *,nsBoxLayoutState &) [layout/xul/base/src/nsSprocketLayout.cpp:1320]
07:01:45     INFO -  nsBoxFrame::GetPrefSize(nsBoxLayoutState &) [layout/xul/base/src/nsBoxFrame.cpp:757]
07:01:45     INFO -  nsSprocketLayout::PopulateBoxSizes(nsIFrame *,nsBoxLayoutState &,nsBoxSize * &,int &,int &,int &) [layout/xul/base/src/nsSprocketLayout.cpp:738]
07:01:45     INFO -  nsSprocketLayout::Layout(nsIFrame *,nsBoxLayoutState &) [layout/xul/base/src/nsSprocketLayout.cpp:221]
07:01:45     INFO -  nsBoxFrame::DoLayout(nsBoxLayoutState &) [layout/xul/base/src/nsBoxFrame.cpp:899]
07:01:45     INFO -  nsIFrame::Layout(nsBoxLayoutState &) [layout/xul/base/src/nsBox.cpp:512]
07:01:45     INFO -  nsStackLayout::Layout(nsIFrame *,nsBoxLayoutState &) [layout/xul/base/src/nsStackLayout.cpp:343]
07:01:45     INFO -  nsBoxFrame::DoLayout(nsBoxLayoutState &) [layout/xul/base/src/nsBoxFrame.cpp:899]
07:01:45     INFO -  nsIFrame::Layout(nsBoxLayoutState &) [layout/xul/base/src/nsBox.cpp:512]
07:01:45     INFO -  nsBoxFrame::Reflow(nsPresContext *,nsHTMLReflowMetrics &,nsHTMLReflowState const &,unsigned int &) [layout/xul/base/src/nsBoxFrame.cpp:703]
07:01:45     INFO -  nsContainerFrame::ReflowChild(nsIFrame *,nsPresContext *,nsHTMLReflowMetrics &,nsHTMLReflowState const &,int,int,unsigned int,unsigned int &,nsOverflowContinuationTracker *) [layout/generic/nsContainerFrame.cpp:971]
07:01:45     INFO -  ViewportFrame::Reflow(nsPresContext *,nsHTMLReflowMetrics &,nsHTMLReflowState const &,unsigned int &) [layout/generic/nsViewportFrame.cpp:226]
07:01:45     INFO -  PresShell::ProcessReflowCommands(bool) [layout/base/nsPresShell.cpp:7929]
07:01:45     INFO -  PresShell::FlushPendingNotifications(mozilla::ChangesToFlush) [layout/base/nsPresShell.cpp:3891]
07:01:45     INFO -  nsRefreshDriver::Tick(__int64,mozilla::TimeStamp) [layout/base/nsRefreshDriver.cpp:962]
07:01:45     INFO -  mozilla::RefreshDriverTimer::TickDriver(nsRefreshDriver *,__int64,mozilla::TimeStamp) [layout/base/nsRefreshDriver.cpp:168]
07:01:45     INFO -  mozilla::RefreshDriverTimer::Tick() [layout/base/nsRefreshDriver.cpp:159]
07:01:45     INFO -  nsTimerImpl::Fire() [xpcom/threads/nsTimerImpl.cpp:547]
07:01:45     INFO -  nsTimerEvent::Run() [xpcom/threads/nsTimerImpl.cpp:636]
07:01:45     INFO -  nsThread::ProcessNextEvent(bool,bool *) [xpcom/threads/nsThread.cpp:627]
Group: core-security
Getting caption button dims is a real pita - 

http://mxr.mozilla.org/mozilla-central/source/widget/windows/nsUXThemeData.cpp#186

we create a window so that we can query it for the button info. The window has to have it's visibility set for the information to be valid. If anyone has any ideas on how to accomplish this in a less heavy handed way I'd love to hear them.
To work around this we could move the query call to some other location, maybe when the hidden toolkit window gets created.
Assignee: nobody → jmathies
Severity: normal → critical
Assignee: jmathies → nobody
Severity: critical → normal
Assignee: nobody → jmathies
Severity: normal → critical
Can anyone suggest a security rating for this? What are the implications in that area?
Probably pretty bad...
Keywords: sec-critical
It seems the culprit here is nsUXThemeData::UpdateTitlebarInfo which only affects
Windows, afaict.
Component: Layout → Widget: Win32
Keywords: crash
(In reply to Andrew McCreight [:mccr8] from comment #4)
> Probably pretty bad...

So specific! Arbitrary code execution? :-)
Per Andrew, this can cause random memory corruption but it might not be terribly controllable. 

Marking for Trunk, may go back as far as Firefox 13.
(In reply to Al Billings [:abillings] from comment #7)
> Per Andrew, this can cause random memory corruption but it might not be
> terribly controllable. 
> 
> Marking for Trunk, may go back as far as Firefox 13.

I'm pretty sure this stretches back to Firefox 4.0, via bug 575870.
Marking wontfix for b2g18 because this is a Windows widget problem.
Jim, can we get a fix for this based on comment 2's idea?
(In reply to Al Billings [:abillings] from comment #10)
> Jim, can we get a fix for this based on comment 2's idea?

I can investigate an earlier call. A little background:

http://mxr.mozilla.org/mozilla-central/source/widget/windows/nsUXThemeData.cpp#186

This queries for metric info on the titlebar buttons for tab in titlebar support. On non-glass desktops we draw our own window controls and with vista+ glass desktops, we let the dwm draw the command buttons and we carve a section out of persona backgrounds so they don't overlap. To do this right we need the exact dims of the normal window control buttons.

The problem is that you can't query for this information on a window that hasn't been displayed yet, and we need this info early in startup during initial layout. The toolkit window doesn't work since it's always hidden and as such doesn't have the right info. The main browser window won't work as it is created way too late, and is set up early not to draw into the titlebar.

So I needed a fresh window that could display briefly and then be destroyed. I ended up creating a visible but fully transparent window to do this. That's why we call create window here.
Posted patch fixSplinter Review
Test on win7 aerobasic and classic, works as expected. Creating a temp window in nsIWidget's create window call certainly shouldn't be an issue.
Attachment #768891 - Flags: review?
Attachment #768891 - Flags: review? → review?(netzen)
Attachment #768891 - Flags: review?(netzen) → review+
Thanks for fixing this.  In the future, you should get sec-approval before landing sec-high or sec-critical bugs that affect more than trunk.
https://hg.mozilla.org/mozilla-central/rev/5cd49ff35fb9
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
(In reply to Phil Ringnalda (:philor) from comment #15)
> https://hg.mozilla.org/mozilla-central/rev/5cd49ff35fb9

Since we just did an uplift, lets give this a week or two and then consider uplifting to aurora and possibly beta.
And esr17?
Depends on: 888765
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #17)
> And esr17?

Is esr 17 released? If so I'm not sure we would do anything about it.
Comment on attachment 768891 [details] [diff] [review]
fix

[Approval Request Comment]
Bug caused by (feature/regressing bug #): something that landed ages ago.
User impact if declined: none.
Testing completed (on m-c, etc.): yep. Note this should include follow up fix in bug 888765.
Risk to taking this patch (and alternatives if risky): seems low based on mc testing that's been done. Flaws would be pretty visible to users.
String or IDL/UUID changes made by this patch: None.

Since this has been open for quite some time without issue I think we can uplift to aurora and let it roll out with the trains from there.
Attachment #768891 - Flags: approval-mozilla-aurora?
In fact we could probably just let it roll out without uplift if we want. I image sec would have input on this decision.
Attachment #768891 - Flags: sec-approval?
(In reply to Jim Mathies [:jimm] from comment #18)
> Is esr 17 released? If so I'm not sure we would do anything about it.

What do you mean? Of course it is.
http://www.mozilla.org/en-US/firefox/organizations/all.html
Comment on attachment 768891 [details] [diff] [review]
fix

This already landed on trunk, so sec-approval doesn't matter any more.
Attachment #768891 - Flags: sec-approval?
I guess you are looking for input on where to land this, in comment 20.
Flags: needinfo?(abillings)
(In reply to Andrew McCreight [:mccr8] from comment #23)
> I guess you are looking for input on where to land this, in comment 20.

yep.
I'm not sure why I got needinfo'd here, Andrew.


(In reply to Jim Mathies [:jimm] from comment #18)
> (In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #17)
> > And esr17?
> 
> Is esr 17 released? If so I'm not sure we would do anything about it.

 ESR17 is a fork of Firefox 17 that contains most of the sec-high and sec-critical fixes (plus a few other things) since then.

It would be nice if someone answered the sec-approval? template questions so we know more about taking this on branches or not (since that requires approval but then trunk did too...).
Flags: needinfo?(abillings)
(In reply to Al Billings [:abillings] from comment #25)
> I'm not sure why I got needinfo'd here, Andrew.

We're trying to decide if this should be uplifted. 

> It would be nice if someone answered the sec-approval? template questions so
> we know more about taking this on branches or not (since that requires
> approval but then trunk did too...).

* How easily could an exploit be constructed based on the patch?

Unknown, it happens very early on startup before content is loaded, so probably pretty minimal.

* Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

No.

* Which older supported branches are affected by this flaw?

See affected flags.

* If not all supported branches, which bug introduced the flaw?

Bug 748156

* Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

minimal effort.

* How likely is this patch to cause regressions; how much testing does it need?

A respectable amount of bake time on mc (15 days) plus one follow up have already been completed. Fallout would be pretty obvious to users, so very little testing is needed. Confirming command buttons have the correct dimensions on classic / basic desktops should suffice.
correction:

* If not all supported branches, which bug introduced the flaw?

bug 575870
Jim: you said this only happens at startup? Does that include the assertion in comment 0? Over in bug 748156 the dangerous assertion happened after we were up to DOMWINDOW == 68.

If this is truly only a startup issue (not that the fix is at startup, but that the resulting problems are) then we can lower this to sec-moderate and not worry about older branches (ESR and Beta 23). Otherwise we should backport it (and bug 888765).
Flags: needinfo?(jmathies)
(In reply to Daniel Veditz [:dveditz] from comment #28)
> Jim: you said this only happens at startup? Does that include the assertion
> in comment 0? Over in bug 748156 the dangerous assertion happened after we
> were up to DOMWINDOW == 68.

Unfortunately there are only five occurrences for that test and the logs are all gone. Then it switched to another test that was much closer to startup.

UpdateTitlebarInfo was called the first time layout asked for button metrics for any of the following non-client command buttons:

NS_THEME_WINDOW_BUTTON_MAXIMIZE
NS_THEME_WINDOW_BUTTON_RESTORE
NS_THEME_WINDOW_BUTTON_MINIMIZE
NS_THEME_WINDOW_BUTTON_CLOSE
NS_THEME_WINDOW_BUTTON_BOX
NS_THEME_WINDOW_BUTTON_BOX_MAXIMIZED

If a set of tests ran without non-client chrome these values would not get queried until a regular chrome window was created. reftest do exactly that, so I'm guessing at some point a crash/ref test created a chromed window for something, hence the late query / assert.
Flags: needinfo?(jmathies)
OK, based on that we should be OK leaving this out of Beta/ESR.
Comment on attachment 768891 [details] [diff] [review]
fix

Approving on aurora and adding qawanted to help with testing per comment #26.
Attachment #768891 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Question for Jim because of Bhavana's QA request in comment 31:

"Confirming command buttons have the correct dimensions on classic / basic desktops should suffice."

Jim, can you give examples of command buttons, and what a classic or basic desktop means? Sorry if that sounds obtuse, I just want to be sure. Thanks.
(In reply to Matt Wobensmith from comment #32)
> Question for Jim because of Bhavana's QA request in comment 31:
> 
> "Confirming command buttons have the correct dimensions on classic / basic
> desktops should suffice."
> 
> Jim, can you give examples of command buttons, and what a classic or basic
> desktop means? Sorry if that sounds obtuse, I just want to be sure. Thanks.

We render the minimize, maximize, and close button in the window chrome on the upper right ourselves on "basic" themes. So in testing on Win7, try switching desktop themes from glass to aero basic or classic and check that those buttons look right. (Note, switching themes while fx is running is broken, so switch before starting firefox.)

With Win7 glass we leave rendering to Windows, but apply special treatment when a persona is installed. Try installing a persona on a glass desktop and make sure the transparent aero buttons aren't overlapping the light weight theme texture in the titlebar.

Also, try enabling/disabling the fx button while on "basic" themes. (alt-view-toolbars-enable/disable the menu bar.)
Thanks Jim.

I ran through all of the scenarios Jim suggested in the previous comment on a pre-patch build, and didn't see any weirdness or crash.

I then did the same on today's m-c build, and again, all seems fine.

I did see artifacts when a persona is installed, and we are switching themes without restarting the browser, but Jim already mentioned that would be expected.

Based on that, I am going to mark this verified for FF25.
Group: layout-core-security
Whiteboard: [adv-main24+]
Verified, same steps, FF24b6.
Status: RESOLVED → VERIFIED
Keywords: verifyme
Group: core-security
You need to log in before you can comment on or make changes to this bug.