Closed
Bug 873073
Opened 11 years ago
Closed 11 years ago
nsNativeThemeWin seems to spin event loop during layout
Categories
(Core :: Widget: Win32, defect)
Tracking
()
People
(Reporter: smaug, Assigned: jimm)
References
Details
(Keywords: assertion, crash, sec-moderate, Whiteboard: [adv-main24+])
Attachments
(1 file)
4.61 KB,
patch
|
bbondy
:
review+
bajaj
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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]
Reporter | ||
Updated•11 years ago
|
Group: core-security
Assignee | ||
Comment 1•11 years ago
|
||
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.
Assignee | ||
Comment 2•11 years ago
|
||
To work around this we could move the query call to some other location, maybe when the hidden toolkit window gets created.
Updated•11 years ago
|
Assignee: nobody → jmathies
Severity: normal → critical
Updated•11 years ago
|
Assignee: jmathies → nobody
Severity: critical → normal
Updated•11 years ago
|
Assignee: nobody → jmathies
Severity: normal → critical
Comment 3•11 years ago
|
||
Can anyone suggest a security rating for this? What are the implications in that area?
Comment 5•11 years ago
|
||
It seems the culprit here is nsUXThemeData::UpdateTitlebarInfo which only affects Windows, afaict.
Component: Layout → Widget: Win32
Keywords: crash
Comment 6•11 years ago
|
||
(In reply to Andrew McCreight [:mccr8] from comment #4) > Probably pretty bad... So specific! Arbitrary code execution? :-)
Comment 7•11 years ago
|
||
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.
status-firefox24:
--- → affected
tracking-firefox24:
--- → +
Assignee | ||
Comment 8•11 years ago
|
||
(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.
Comment 9•11 years ago
|
||
Marking wontfix for b2g18 because this is a Windows widget problem.
status-b2g18:
--- → wontfix
status-firefox21:
--- → wontfix
status-firefox22:
--- → affected
status-firefox23:
--- → affected
status-firefox-esr17:
--- → affected
Comment 10•11 years ago
|
||
Jim, can we get a fix for this based on comment 2's idea?
Updated•11 years ago
|
Assignee | ||
Comment 11•11 years ago
|
||
(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.
Updated•11 years ago
|
status-firefox25:
--- → affected
Assignee | ||
Comment 12•11 years ago
|
||
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?
Assignee | ||
Updated•11 years ago
|
Attachment #768891 -
Flags: review? → review?(netzen)
Updated•11 years ago
|
Attachment #768891 -
Flags: review?(netzen) → review+
Assignee | ||
Comment 13•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/5cd49ff35fb9 thanks for the quick review!
Comment 14•11 years ago
|
||
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.
Comment 15•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/5cd49ff35fb9
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Assignee | ||
Comment 16•11 years ago
|
||
(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.
Comment 17•11 years ago
|
||
And esr17?
Assignee | ||
Comment 18•11 years ago
|
||
(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.
Assignee | ||
Comment 19•11 years ago
|
||
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?
Assignee | ||
Comment 20•11 years ago
|
||
In fact we could probably just let it roll out without uplift if we want. I image sec would have input on this decision.
Assignee | ||
Updated•11 years ago
|
Attachment #768891 -
Flags: sec-approval?
Comment 21•11 years ago
|
||
(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 22•11 years ago
|
||
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?
Comment 23•11 years ago
|
||
I guess you are looking for input on where to land this, in comment 20.
Flags: needinfo?(abillings)
Assignee | ||
Comment 24•11 years ago
|
||
(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.
Comment 25•11 years ago
|
||
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)
Assignee | ||
Comment 26•11 years ago
|
||
(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.
Assignee | ||
Comment 27•11 years ago
|
||
correction: * If not all supported branches, which bug introduced the flaw? bug 575870
Comment 28•11 years ago
|
||
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).
Assignee | ||
Comment 29•11 years ago
|
||
(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)
Comment 30•11 years ago
|
||
OK, based on that we should be OK leaving this out of Beta/ESR.
Keywords: sec-high → sec-moderate
Updated•11 years ago
|
tracking-firefox25:
--- → +
Updated•11 years ago
|
Updated•11 years ago
|
Comment 31•11 years ago
|
||
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+
Comment 32•11 years ago
|
||
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.
Assignee | ||
Comment 33•11 years ago
|
||
(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.)
Comment 34•11 years ago
|
||
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.
Updated•11 years ago
|
Group: layout-core-security
Updated•11 years ago
|
Whiteboard: [adv-main24+]
Comment 36•11 years ago
|
||
Verified, same steps, FF24b6.
Updated•10 years ago
|
Group: core-security
You need to log in
before you can comment on or make changes to this bug.
Description
•