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.

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...

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.

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?

> 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.

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.

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).

(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.

Bug 785626 is a partial backout of bug 785333. The rest of bug 785333 only landed in branch 18.

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)

As TestPilot is the culprit, bug 785626 is no longer suspected.

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?

(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?

Sorry, I should have passed this off to someone ... we're at a workweek and I spaced. Sending over to ilana.

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.

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.

Regression range http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=86ee4deea55b&tochange=50e4ff05741e

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.

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.

Which popup is being hidden here?

The Test Pilot notification popup.

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.

This makes us send activate and deactivate notifications to the toplevel window, like we used to before bug 743975, instead of the panel.

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.

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.

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.

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.

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.

(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.