Closed Bug 787818 Opened 7 years ago Closed 7 years ago

crash in nsXULPopupManager::HidePopupCallback @ nsMenuPopupFrame::HidePopup with TestPilot

Categories

(Core :: Layout, defect, critical)

17 Branch
x86
Windows 7
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla19
Tracking Status
firefox17 + wontfix
firefox18 + fixed
firefox19 + fixed
firefox-esr10 --- unaffected
firefox-esr17 - wontfix

People

(Reporter: scoobidiver, Assigned: tnikkel)

References

Details

(4 keywords, Whiteboard: [adv-main18+])

Crash Data

Attachments

(4 files)

It's a low volume crash in 15.0, but much higher in 17.0a2 where it's #14 top browser crasher.

Signature 	nsMenuPopupFrame::HidePopup(bool, nsPopupState) More Reports Search
UUID	c1acceb4-a6e9-444f-a5bb-f936b2120902
Date Processed	2012-09-02 17:20:07
Uptime	735
Install Age	1.3 hours since version was first installed.
Install Time	2012-09-02 16:04:00
Product	Firefox
Version	17.0a2
Build ID	20120901042009
Release Channel	aurora
OS	Windows NT
OS Version	5.1.2600 Dodatek Service Pack 3
Build Architecture	x86
Build Architecture Info	AuthenticAMD family 16 model 4 stepping 2
Crash Reason	EXCEPTION_ACCESS_VIOLATION_EXEC
Crash Address	0x616e616d
App Notes 	
AdapterVendorID: 0x1002, AdapterDeviceID: 0x9442, AdapterSubsysID: 040110b0, AdapterDriverVersion: 8.970.100.3000
D3D10 Layers? D3D10 Layers- D3D9 Layers? D3D9 Layers+ 
EMCheckCompatibility	True
Adapter Vendor ID	0x1002
Adapter Device ID	0x9442
Total Virtual Memory	2147352576
Available Virtual Memory	1833201664
System Memory Use Percentage	21
Available Page File	4861616128
Available Physical Memory	2730385408

Frame 	Module 	Signature 	Source
0 		@0x616e616d 	
1 	xul.dll 	nsMenuPopupFrame::HidePopup 	layout/xul/base/src/nsMenuPopupFrame.cpp:795
2 	xul.dll 	nsXULPopupManager::HidePopupCallback 	layout/xul/base/src/nsXULPopupManager.cpp:879
3 	xul.dll 	nsXULPopupManager::FirePopupHidingEvent 	layout/xul/base/src/nsXULPopupManager.cpp:1231
4 	xul.dll 	nsXULElement::AddRef 	content/xul/content/src/nsXULElement.cpp:312
5 	mozjs.dll 	JS_GetFrameFunctionObject 	js/src/jsdbgapi.cpp:597
6 	xul.dll 	nsPopupBoxObject::HidePopup 	layout/xul/base/src/nsPopupBoxObject.cpp:49
7 	xul.dll 	NS_InvokeByIndex_P 	xpcom/reflect/xptcall/src/md/win32/xptcinvoke.cpp:70
8 	xul.dll 	XPCNativeMember::Resolve 	js/xpconnect/src/XPCWrappedNativeInfo.cpp:102
9 	xul.dll 	nsXPConnect::GetXPConnect 	js/xpconnect/src/nsXPConnect.cpp:139
10 	xul.dll 	XPCWrappedNative::GetAttribute 	js/xpconnect/src/xpcprivate.h:2822
11 	gkmedias.dll 	define_gf_group 	media/libvpx/vp8/encoder/firstpass.c:1800

More reports at:
https://crash-stats.mozilla.com/report/list?signature=nsMenuPopupFrame%3A%3AHidePopup%28bool%2C+nsPopupState%29
More reports also at:
https://crash-stats.mozilla.com/report/list?signature=nsView%3A%3ANotifyEffectiveVisibilityChanged%28bool%29

With combined signatures, it's even #8 top browser crasher in 17.0a2.
Crash Signature: [@ nsMenuPopupFrame::HidePopup(bool, nsPopupState)] → [@ nsMenuPopupFrame::HidePopup(bool, nsPopupState)] [@ nsView::NotifyEffectiveVisibilityChanged(bool)]
Crash Signature: [@ nsMenuPopupFrame::HidePopup(bool, nsPopupState)] [@ nsView::NotifyEffectiveVisibilityChanged(bool)] → [@ nsMenuPopupFrame::HidePopup(bool, nsPopupState)] [@ nsView::NotifyEffectiveVisibilityChanged(bool)] [@ nsView::DropMouseGrabbing()]
795: viewManager->ResizeView(view, nsRect(0, 0, 0, 0));

Fwiw, bug 786421 removed that particular line a few days ago.
(In reply to Mats Palmgren [:mats] from comment #2)
> 795: viewManager->ResizeView(view, nsRect(0, 0, 0, 0));
> 
> Fwiw, bug 786421 removed that particular line a few days ago.

The set of crashes in comment 1 crash on the line just before, so removing that line doesn't seem to help us here. Looks like the view manager pointer is invalid?
I clicked through 20 or so crashes and I didn't see one that is on the
line before.  There seems to be two sets of crashes:

The first set is on the "viewManager->ResizeView" line, these usually have
a crash addresses NOT near zero: 0x616e616d, 0x857cd43, 0x4bdaac2, 0x6f7365b6
Most of these are for 17.0a2:
bp-9a5aac85-2c13-486c-846e-59e262120830
bp-52df2bba-e295-48f1-b9bd-4b8d92120901

The second set crash one or two lines after ResizeView:
  FireDOMEvent(NS_LITERAL_STRING("DOMMenuInactive"), mContent);
or
  nsEventStates state = mContent->AsElement()->State();
and usually have a crash address near zero: 0x15, 0x30, 0x4
bp-ccfcbc57-239b-4816-9d67-b45c82120830
bp-6582bb5e-fb27-40be-b8b0-4dfcb2120828
Most of these are for 15.0 or older.

If the viewManager/view is bogus already in HidePopup I would expect the
first set to crash on the SetViewVisibility call.  So, could it be the
ResizeView call that leads up to a layout flush?
Ah, now I see that you said comment 1.  (I looked at the set in comment 0).
Right, so there's a third set.  Seems like there could be more than one bug
here.
(In reply to Mats Palmgren [:mats] from comment #4)
> bp-ccfcbc57-239b-4816-9d67-b45c82120830
> bp-6582bb5e-fb27-40be-b8b0-4dfcb2120828
> Most of these are for 15.0 or older.
Those crashes are fixed in 16.0 and above, that is why I haven't post their stack traces.

In addition to those of comment 0 and comment 1, more reports also at:
https://crash-stats.mozilla.com/report/list?signature=nsView%3A%3ADropMouseGrabbing%28%29
Hiding this bug since I don't want to speculate about causes in the public...
Group: core-security
There's a few nsEventDispatcher::Dispatch calls in nsXULPopupManager.cpp, e.g.
http://mxr.mozilla.org/mozilla-central/source/layout/xul/base/src/nsXULPopupManager.cpp#1198
I think we need to check that all objects we touch are still alive after those.
After the one in FirePopupHidingEvent, presShell->IsDestroying() could be
true, which could be the cause of the bad View/ViewManager.
Attached patch weakframe checkSplinter Review
So I checked all frames and content nodes and I think this is the only possibly bad use of a frame and I think all content nodes have nsCOMPtr's holding onto them (possibly in the calling function, which isn't ideal).

Do you think we also need to check if the presshell is destroying after unsafe calls?
Attachment #658151 - Flags: review?(matspal)
> So I checked all frames and content nodes and I think this is the only
> possibly bad use of a frame and I think all content nodes have nsCOMPtr's
> holding onto them (possibly in the calling function, which isn't ideal).

OK, I'm looking through nsXULPopupManager.cpp and I'm worried that
nsMenuChainItem::mFrame might be stale.  E.g.
HidePopupsInList
  HidePopup
    FirePopupHidingEvent
      HidePopupCallback
        Dispatch
          * destroys the world *
* unwind to HidePopupsInList *
  SetCaptureState(nullptr)
    item = GetTopVisibleMenu()
    popup = item->Frame();
    * popup is destroyed *

Also applies to calls to item->Content():
nsIContent* nsMenuChainItem::Content()
{
  return mFrame->GetContent();
}

so we need to check all methods that can reach Dispatch if they use
a nsMenuChainItem after such a call.

> Do you think we also need to check if the presshell is destroying after
> unsafe calls?

Probably not necessary, PresShell::Destroy nulls out its VM pointer.
(In reply to Mats Palmgren [:mats] from comment #10)
> OK, I'm looking through nsXULPopupManager.cpp and I'm worried that
> nsMenuChainItem::mFrame might be stale.  E.g.

Ah, good point. I assumed that the lifetime management of those items was not suspect so I didn't give them any scrutiny.
nsIContent* nsXULPopupManager::Rollup(...)
{
  nsIContent* lastRolledUpPopup;
  // ... assign lastRolledUpPopup ...
  HidePopup() // reach Dispatch
  return lastRolledUpPopup;
}

what is keeping lastRolledUpPopup alive?
Comment on attachment 658151 [details] [diff] [review]
weakframe check

This looks good, and is likely the cause of the crash at hand.
We can investigate the other issues separately if you wish.
Attachment #658151 - Flags: review?(matspal) → review+
Thanks for the review. Yeah, I think we should land this clearly correct fix ASAP and then look at the other issues.
(In reply to Mats Palmgren [:mats] from comment #12)
> nsIContent* nsXULPopupManager::Rollup(...)
> {
>   nsIContent* lastRolledUpPopup;
>   // ... assign lastRolledUpPopup ...
>   HidePopup() // reach Dispatch
>   return lastRolledUpPopup;
> }
> 
> what is keeping lastRolledUpPopup alive?

My somewhat narrow search didn't look at all HidePopup callers so I didn't find this issue.
Comment on attachment 658151 [details] [diff] [review]
weakframe check

We don't actually use the weak frame later in this function. So maybe I was too hasty in this patch. Still doesn't hurt anything though.
Whiteboard: [leave open]
It guarantees that the pres shell is alive, and more importantly
that all its ancestor frames are alive, which are used through
mPopups later in the method.  (Assuming that a nsMenuChainItem
ancestor also is a frame ancestor).
https://hg.mozilla.org/mozilla-central/rev/f182ead52cd9
Assignee: nobody → tnikkel
Flags: in-testsuite?
Target Milestone: --- → mozilla18
(In reply to Mats Palmgren [:mats] from comment #10)
> > So I checked all frames and content nodes and I think this is the only
> > possibly bad use of a frame and I think all content nodes have nsCOMPtr's
> > holding onto them (possibly in the calling function, which isn't ideal).
> 
> OK, I'm looking through nsXULPopupManager.cpp and I'm worried that
> nsMenuChainItem::mFrame might be stale.  E.g.
> HidePopupsInList
>   HidePopup
>     FirePopupHidingEvent
>       HidePopupCallback
>         Dispatch
>           * destroys the world *
> * unwind to HidePopupsInList *
>   SetCaptureState(nullptr)
>     item = GetTopVisibleMenu()
>     popup = item->Frame();
>     * popup is destroyed *
> 

Note though that the corresponding nsMenuChainItems are removed from the mFrames/mNoHidePanels lists within PopupDestroyed when their frames are destroyed so the call to GetTopVisibleMenu won't iterate over any destroyed items.
Ah, good point.  We still need to be very careful about local
nsMenuChainItem* on the stack though.

OK, so the patch that landed is redundant -- then I don't see
what could have caused the crash stack, assuming all involved
nsIContent* have a strong ref somewhere.

Neil, do you have a theory for the crash stack?
How about |popupsToHide| in PopupDestroyed():

  nsTArray<nsMenuPopupFrame *> popupsToHide;
  while {
    ...
    HidePopup() // destroys the world
    break;
  }
  HidePopupsInList(popupsToHide, false);

The pointers in it could be stale when we pass it on to HidePopupsInList?
(In reply to Mats Palmgren [:mats] from comment #22)
> How about |popupsToHide| in PopupDestroyed():
> 
>   nsTArray<nsMenuPopupFrame *> popupsToHide;
>   while {
>     ...
>     HidePopup() // destroys the world

HidePopup is called here with aAsynchronous = true, so events fire asynchronously.
There's only one crash in 18.0a1, bp-e0709a31-c4c4-4a6c-9a09-629aa2120903, whereas it's #8 top browser crasher in 17.0a2.
It might be a regression from bug 785626 that landed after the merge but before the first Aurora build in 17.0a2.
Not working on this directly at the moment.
Assignee: tnikkel → nobody
Blocks: 785626
Bug 785626 is a partial backout of bug 785333.
The rest of bug 785333 only landed in branch 18.
Crash Signature: [@ nsMenuPopupFrame::HidePopup(bool, nsPopupState)] [@ nsView::NotifyEffectiveVisibilityChanged(bool)] [@ nsView::DropMouseGrabbing()] → [@ nsMenuPopupFrame::HidePopup(bool, nsPopupState)] [@ nsView::NotifyEffectiveVisibilityChanged(bool)] [@ nsView::DropMouseGrabbing()] [@ nsAsyncDOMEvent::nsAsyncDOMEvent(nsINode*, nsAString_internal const&, bool, bool)]
Duplicate of this bug: 790396
It's #2 top browser crasher in 17.0a2 and accounts for 11% of all crashes.

It's related to TestPilot according to comments (survey popup) and correlations:
  nsAsyncDOMEvent::nsAsyncDOMEvent(nsINode*, nsAString_internal const&, bool, bool)|EXCEPTION_ACCESS_VIOLATION_READ (329 crashes)
     99% (327/329) vs.  81% (3846/4760) testpilot@labs.mozilla.com (Mozilla Labs - Test Pilot, https://addons.mozilla.org/addon/13661)
  nsView::NotifyEffectiveVisibilityChanged(bool)|EXCEPTION_ACCESS_VIOLATION_READ (272 crashes)
     99% (268/272) vs.  81% (3846/4760) testpilot@labs.mozilla.com (Mozilla Labs - Test Pilot, https://addons.mozilla.org/addon/13661)
  nsView::DropMouseGrabbing()|EXCEPTION_ACCESS_VIOLATION_READ (96 crashes)
    100% (96/96) vs.  81% (3846/4760) testpilot@labs.mozilla.com (Mozilla Labs - Test Pilot, https://addons.mozilla.org/addon/13661)
Summary: crash in nsXULPopupManager::HidePopupCallback @ nsMenuPopupFrame::HidePopup → crash in nsXULPopupManager::HidePopupCallback @ nsMenuPopupFrame::HidePopup with TestPilot
As TestPilot is the culprit, bug 785626 is no longer suspected.
No longer blocks: 785626
Now that version 18 is in Aurora with TestPilot, it's affected.
With combined signatures, it's #7 top crasher in 17.0b1 and #2 in 18.0a2.
(In reply to Scoobidiver from comment #30)
> Now that version 18 is in Aurora with TestPilot, it's affected.
> With combined signatures, it's #7 top crasher in 17.0b1 and #2 in 18.0a2.

Jet - this is really hurting our TestPilot users on Aurora/Beta. Would you mind finding an assignee?
Assignee: nobody → bugs
(In reply to Scoobidiver from comment #30)
> Now that version 18 is in Aurora with TestPilot, it's affected.
> With combined signatures, it's #7 top crasher in 17.0b1 and #2 in 18.0a2.

Same correlation on Beta?
(In reply to Alex Keybl [:akeybl] from comment #32)
> Same correlation on Beta?
Yes. Here they are:
  nsView::NotifyEffectiveVisibilityChanged(bool)|EXCEPTION_ACCESS_VIOLATION_READ (138 crashes)
     97% (134/138) vs.  82% (32437/39681) testpilot@labs.mozilla.com (Mozilla Labs - Test Pilot, https://addons.mozilla.org/addon/13661)

  nsAsyncDOMEvent::nsAsyncDOMEvent(nsINode*, nsAString_internal const&, bool, bool)|EXCEPTION_ACCESS_VIOLATION_READ (101 crashes)
     97% (98/101) vs.  82% (32437/39681) testpilot@labs.mozilla.com (Mozilla Labs - Test Pilot, https://addons.mozilla.org/addon/13661)
      8% (8/101) vs.   2% (717/39681) mozilla_cc@internetdownloadmanager.com (IDM CC, https://addons.mozilla.org/addon/6973)

  nsView::DropMouseGrabbing()|EXCEPTION_ACCESS_VIOLATION_READ (40 crashes)
     95% (38/40) vs.  82% (32437/39681) testpilot@labs.mozilla.com (Mozilla Labs - Test Pilot, https://addons.mozilla.org/addon/13661)
      8% (3/40) vs.   1% (546/39681) firebug@software.joehewitt.com (Firebug, https://addons.mozilla.org/addon/1843)
If it happens with Test Pilot maybe we could play around with Test Pilot and try to find some steps to reproduce?
(In reply to Timothy Nikkel (:tn) from comment #34)
> If it happens with Test Pilot maybe we could play around with Test Pilot and
> try to find some steps to reproduce?
According to comments in 18.0a2, it's easy to reproduce:
"closing a popup notice for a firefox survey"
"I clicked the "X" button to close the notification for the test pilot survey and firefox crashed."
"I was rearranging my toolbar and had just declined to take a survey"
I'm asking the Test pilot team to push out surveys next week (on both 17 and 18).  Should I hold off?
Is there a way to get the survey to appear so that this bug can be reproduced?
a lot of the nsMenuFrame::HidePopup crashes in 17.0a2 were at high addresses and EXCEPTION_ACCESS_VIOLATION_EXEC -- definitely exploitable if we could reproduce it, but they're all clustered around reporting date of Sept 26 - Oct 2. Does that correspond with a Test Pilot survey?  The build dates were from a month prior to that, Aug 31 - Sept 4. Who keeps their Aurora builds that out of date?

The overwhelming majority of the crashes at nsView::NotifyEffectiveVisibilityChanged(bool) were
EXCEPTION_ACCESS_VIOLATION_READ at 0x72657397 -- always that address, across different versions of windows and different Firefox builds (but all 17 or 18). Very suspicious to have such a fixed address, unless it's data. "res<?>" ?
(In reply to [:Cww] from comment #36)
> I'm asking the Test pilot team to push out surveys next week (on both 17 and
> 18).  Should I hold off?

If we got actionable results from the last survey (similarly affected), we shouldn't block on this.
(In reply to Neil Deakin from comment #37)
> Is there a way to get the survey to appear so that this bug can be
> reproduced?

Gregg - can you advise Neil here?
Can someone from this team schedule a bit of phone time tomorrow (glind@moz will get my calendar) on this.  I don't really understand the issues here.  Thanks!
(In reply to Gregg Lind (User Research - Test Pilot) from comment #41)
> Can someone from this team schedule a bit of phone time tomorrow (glind@moz
> will get my calendar) on this.  I don't really understand the issues here. 
> Thanks!

Neil would like to figure out how to trigger a test pilot survey, to reproduce this crash.
Cheng - do you know the answer to Commment 42?
Flags: needinfo?(cwwmozilla)
Sorry, I should have passed this off to someone ... we're at a workweek and I spaced. Sending over to ilana.
Flags: needinfo?(cwwmozilla)
Flags: needinfo?(isegall)
Cheng, I guess this work week is over, and we still have no reply from you or Ilana on the tracking+ bug for 17.
This has resided in 17, though, I guess because we are not running a Testpilot survey right now, but I see it at #9 in 18 data from yesterday. We really should move forward here.
We will have to keep chasing this down for 18, but at this point we're too late for 17.
I'm confused, Ilana sent an email to a lot of people offering help to work on it but it seems to have gotten dropped.

Hopefully, it can make 18. We're deliberately not running anything on test pilot (which means no aurora survey this cycle) as we're holding on this bug.
Flags: needinfo?(isegall)
Duplicate of this bug: 810158
This is showing up on the explosive report today for FF 17 - https://crash-analysis.mozilla.com/rkaiser/2012-11-09/2012-11-09.firefox.17.explosiveness.html - although volume wise for the week it only has 542 crashes across all versions.
The 542 crashes I cited in comment was for the nsMenuPopupFrame::HidePopup(bool, nsPopupState) signature.  nsView::NotifyEffectiveVisibilityChanged(bool) has 2327 in the last week across all versions, so when you start adding these signatures up - 771 for nsView::DropMouseGrabbing() and the final signature has 1329 in the last week - that totals to almost 5K crashes in the last week.
Okay, I am free anytime until EOD today, or tomorrow to work through this.  

http://hg.mozilla.org/labs/testpilotweb/raw-file/tip/README.md  is our debug procedure, but it will be much easier to work with someone in real-time to reproduce this.  

Thanks!

Gregg
I am practicing with my local copies of everything and am unable to reproduce this bug.
This should help:  (also, `testpilothelper` above is an alternate path)

#. edit setting to fake version of the testcases directory
extensions.testpilot.indexBaseURL -> https://people.mozilla.com/~glind/onesurvey/testcases/ 

#. Restart (sorry!)

#. Open TP Debug page
chrome://testpilot/content/debug.html

#. Error console
chrome://global/content/console.xul

#. After some hard refreshes, once things load, choose:  
`a_survey_for_testing_crashes` and use the dummy popup button.  

If this is too complicated (which it is!), ask me for help, and we can try to make another pass at simplifying.
Thanks. With the steps from comment 53 I've been able to get a crash two times now, but with a lot of trying. I'm trying to determine steps that more reliably cause a crash.
Oh and I don't get the test survey (a_survey_for_testing_crashes), I only get the real surveys.
Reliable steps: once you get a test pilot popup, bring another window to the foreground, and with that other window still the foreground window click on the submit button on the test pilot notification popup.
Tn,

Poke me on IRC if you want to work this real time.
Tn,

Poke me on IRC if you want to work this real time.  (gregglind)
Thanks Gregg, I think we have everything we need right now to reproduce and debug this. If we need more Test Pilot info we'll let you know.
Attached file stack
nsMenuPopupFrame::HidePopup sets the view visibility to hidden, which calls nsWindow::Show(false), which calls ::ShowWindow() to tell Windows to hide the window. This responds synchronously with a WM_SETFOCUS message on the main toplevel window. And the focus subsystem flushes in CheckIfFocusable and that kills the menupopup frame because the onPopupHiding event handler for the event we dispatch just a little earlier made some changes that required a frame reconstruct of the popup.
So if calling nsWindow::Show can flush and destroy anything we are going to need to add protections in a lot of places.

But I'm suspicious as to why this only started crashing with the regression range in comment 57. I think perhaps http://hg.mozilla.org/mozilla-central/diff/448410c2035e/widget/windows/nsWindow.cpp from bug 743975 could have been what triggered this, but I don't see how.
Assignee: bugs → tnikkel
Which popup is being hidden here?
The Test Pilot notification popup.
Attached file Testcase
To reproduce:

1. Open as a chrome window.
2. Press the Open button.
3. Click on another application.
4. Without focusing the testcase window again, press the Close button.

Note that the mouse click fires before the window is raised.

It's possible that bug 743975 changed the event firing in some manner that caused the focus event to now fire when it didn't before.
I think so, in older builds I think clicking on the button in the popup when another application is foreground would bring Firefox and the popup to the front but the button wouldn't get a click event.
The change that http://hg.mozilla.org/mozilla-central/diff/448410c2035e/widget/windows/nsWindow.cpp made that causes this is that nsWindow::DispatchFocusToTopLevelWindow now finds the top level window and then dispatches the active/deactivate to our mWidgetListener and not the mWidgetListener of the top level window we just found. I'll post a patch tomorrow.
Blocks: 743975
Attached patch patchSplinter Review
This makes us send activate and deactivate notifications to the toplevel window, like we used to before bug 743975, instead of the panel.
Attachment #682552 - Flags: review?(enndeakin)
Attachment #682552 - Flags: review?(enndeakin) → review+
Comment on attachment 682552 [details] [diff] [review]
patch

[Security approval request comment]
How easily can the security issue be deduced from the patch?
I don't think it's easy at all.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
No, I don't think so.

Which older supported branches are affected by this flaw?
version 17 through to current.

If not all supported branches, which bug introduced the flaw?
This is a regression from bug 743975.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
The patch should apply directly, if not it will be very simple, the code hasn't change since the original regression.

How likely is this patch to cause regressions; how much testing does it need?
It restores us to the behaviour we had pre bug 743975, I don't think it needs extensive testing.
Attachment #682552 - Flags: sec-approval?
We're releasing Firefox 17 on tuesday and it is affected by this bug. I don't think it's reasonable to land this patch in Firefox 17, which is sad because it will be the only release to be affected by this bug.
Whiteboard: [leave open]
lowering security severity, it looks like there's no way for web content to trigger this otherwise exploitable condition.
Comment on attachment 682552 [details] [diff] [review]
patch

sec-approval+ for mozilla-central.
Attachment #682552 - Flags: sec-approval? → sec-approval+
https://hg.mozilla.org/mozilla-central/rev/094819a5ee7a
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
The patch here means that we no longer hit a bad case which destroys us when calling nsWindow::Show (via setting view visibility). The badness happens because Windows sends us back a focus notification in response to hiding a window. There is no obvious reason why Windows can't send us a focus notification when we are hiding or showing a window, and so if that is true there are a lot of other places that we should guard against to be safe from this type of problem. However the fact that we've never been bitten by this before makes me wonder if there is a reason for that.
Comment on attachment 682552 [details] [diff] [review]
patch

Can we please have it in beta

User impact if declined: We can't run test pilot studies which puts us back 6 weeks on some key studies for this quarter and general crashiness.
Attachment #682552 - Flags: approval-mozilla-beta?
Yeah, I was going to request approval today, just wanted a bit of time on nightly to catch any regressions. We should take this on beta, it restores our window focus notifications to how we did them prior to bug 743975, so it should be low risk.
Attachment #682552 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(In reply to Timothy Nikkel (:tn) from comment #78)
> Yeah, I was going to request approval today, just wanted a bit of time on
> nightly to catch any regressions. We should take this on beta, it restores
> our window focus notifications to how we did them prior to bug 743975, so it
> should be low risk.

If we land now, we can still make it into beta 1.
Whiteboard: [adv-main18+]
Group: core-security
You need to log in before you can comment on or make changes to this bug.