It first showed up in 21.0a1/20130131. The regression range is: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=677e87c11252&tochange=20bbf73921f4 It's likely a regression from bug 822367. Signature nsSecureBrowserUIImpl::MapInternalToExternalState(unsigned int*, nsSecureBrowserUIImpl::lockIconState, bool) More Reports Search UUID 0af999d0-4d49-4d56-9d08-e30e32130131 Date Processed 2013-01-31 23:30:53 Uptime 12181 Install Age 3.4 hours since version was first installed. Install Time 2013-01-31 20:07:30 Product Firefox Version 21.0a1 Build ID 20130131031009 Release Channel nightly OS Windows NT OS Version 6.1.7601 Service Pack 1 Build Architecture x86 Build Architecture Info GenuineIntel family 6 model 23 stepping 10 Crash Reason EXCEPTION_ACCESS_VIOLATION_READ Crash Address 0x0 App Notes AdapterVendorID: 0x1002, AdapterDeviceID: 0x9598, AdapterSubsysID: 00000000, AdapterDriverVersion: 8.930.0.0 D2D? D2D+ DWrite? DWrite+ D3D10 Layers? D3D10 Layers+ Processor Notes sp-processor09.phx1.mozilla.com_15932:2008 EMCheckCompatibility True Adapter Vendor ID 0x1002 Adapter Device ID 0x9598 Total Virtual Memory 4294836224 Available Virtual Memory 732778496 System Memory Use Percentage 75 Available Page File 9368604672 Available Physical Memory 1560924160 Frame Module Signature Source 0 xul.dll nsSecureBrowserUIImpl::MapInternalToExternalState security/manager/boot/src/nsSecureBrowserUIImpl.cpp:298 1 xul.dll nsSecureBrowserUIImpl::TellTheWorld security/manager/boot/src/nsSecureBrowserUIImpl.cpp:1412 2 xul.dll nsSecureBrowserUIImpl::EvaluateAndUpdateSecurityState security/manager/boot/src/nsSecureBrowserUIImpl.cpp:563 3 xul.dll nsSecureBrowserUIImpl::OnLocationChange security/manager/boot/src/nsSecureBrowserUIImpl.cpp:1497 4 xul.dll nsDocLoader::FireOnLocationChange uriloader/base/nsDocLoader.cpp:1345 5 xul.dll nsDocShell::CreateContentViewer docshell/base/nsDocShell.cpp:8057 6 xul.dll nsDSURIContentListener::DoContent docshell/base/nsDSURIContentListener.cpp:122 7 xul.dll nsDocumentOpenInfo::TryContentListener uriloader/base/nsURILoader.cpp:659 8 xul.dll nsDocumentOpenInfo::DispatchContent uriloader/base/nsURILoader.cpp:360 9 xul.dll nsDocumentOpenInfo::OnStartRequest uriloader/base/nsURILoader.cpp:252 10 xul.dll mozilla::net::nsHttpChannel::CallOnStartRequest netwerk/protocol/http/nsHttpChannel.cpp:960 11 user32.dll CalcWakeMask 12 xul.dll PeekUIMessage widget/windows/nsAppShell.cpp:60 13 nspr4.dll nspr4.dll@0x90d0 14 xul.dll nsInputStreamPump::OnStateStart netwerk/base/src/nsInputStreamPump.cpp:418 15 xul.dll nsInputStreamPump::OnInputStreamReady netwerk/base/src/nsInputStreamPump.cpp:369 16 xul.dll nsInputStreamReadyEvent::Run xpcom/io/nsStreamUtils.cpp:82 17 xul.dll nsThread::ProcessNextEvent xpcom/threads/nsThread.cpp:627 18 xul.dll NS_ProcessNextEvent_P obj-firefox/xpcom/build/nsThreadUtils.cpp:238 19 xul.dll mozilla::ipc::MessagePump::Run ipc/glue/MessagePump.cpp:82 20 xul.dll MessageLoop::RunHandler ipc/chromium/src/base/message_loop.cc:208 21 xul.dll MessageLoop::Run ipc/chromium/src/base/message_loop.cc:182 22 xul.dll nsBaseAppShell::Run widget/xpwidgets/nsBaseAppShell.cpp:163 23 xul.dll nsAppShell::Run widget/windows/nsAppShell.cpp:154 24 xul.dll nsAppStartup::Run toolkit/components/startup/nsAppStartup.cpp:288 25 xul.dll XREMain::XRE_mainRun toolkit/xre/nsAppRunner.cpp:3823 26 xul.dll XREMain::XRE_main toolkit/xre/nsAppRunner.cpp:3890 27 xul.dll XRE_main toolkit/xre/nsAppRunner.cpp:4093 28 firefox.exe do_main browser/app/nsBrowserApp.cpp:185 29 firefox.exe wmain toolkit/xre/nsWindowsWMain.cpp:105 More reports at: https://crash-stats.mozilla.com/report/list?signature=nsSecureBrowserUIImpl%3A%3AMapInternalToExternalState%28unsigned+int*%2C+nsSecureBrowserUIImpl%3A%3AlockIconState%2C+bool%29

bsmith - seems like we might want to go back to returning NS_OK instead of the MOZ_ASSERT -> https://mxr.mozilla.org/mozilla-central/source/security/manager/boot/src/nsSecureBrowserUIImpl.cpp#292 (Per our discussion in comment 62 and 63 here - https://bugzilla.mozilla.org/show_bug.cgi?id=822367#c62)

What are cases where we wouldn't have a docshell here?

URLs: 1 about:newtab 1 http://www.distrukt.com/demo/wp-admin/themes.php?page=ot-theme-options 1 about:blank 1 http://redmine8.aiyra.com/issues/2880

The docshell exists because nsDocShell::CreateContentViewer is called before we get to nsSecureBrowserUIImpl::MapInternalToExternalState. So where did the docshell go? Still looking into this.

It looks like the problem isn't that the docshell "went away" (since, like you said, it is in the stack), but that the window doesn't have a reference to the DocShell. I don't know why that is. In order to know whether it is safe to just "return NS_OK" in this situation, we should know when/why this condition happens.

I've ran into this twice today: 23039c1c-593b-4a54-b366-97f302130204 906ac2b3-d493-4b21-8c94-37e722130204 Common denominator was using ctrl+w to rapidly close multiple tabs then switching tab groups. I'll see if I can get a better repro then that.

assigning to tanvi for now based on comment 4 ("looking into this")

This crash is probably occuring because mWindow is a weak reference - https://mxr.mozilla.org/mozilla-central/source/security/manager/boot/src/nsSecureBrowserUIImpl.cpp#190 Hence, we have to check if pwin exists before we try and get the docshell. r? to bsmith and a push to try: https://tbpl.mozilla.org/?tree=Try&rev=d2e17ab48969

Pushed to inbound: http://hg.mozilla.org/integration/mozilla-inbound/rev/d2353a795fb7

There are still crashes.

(In reply to Brent Garber from comment #6) > I've ran into this twice today: > > 23039c1c-593b-4a54-b366-97f302130204 > 906ac2b3-d493-4b21-8c94-37e722130204 > > Common denominator was using ctrl+w to rapidly close multiple tabs then > switching tab groups. I'll see if I can get a better repro then that. Brent, do you have any extra info here that could help reproduce? Any information you may find relevant would be great: modified preferences (do you have the security.mixed_content.block_active_content pref set to true?), sites you had opened when the crash occurred, Tab preferences, Add-ons/toolbars installed. Not successful in reproducing as of yet. Tried using Panorama, Ctrl-W, Undo closed tabs, sites with mixed content, security related prefs on/off. Mozilla/5.0 (X11; Linux i686; rv:21.0) Gecko/20130210 Firefox/21.0

Addons ---------- Addblock Plus 2.2.2 Firebug 1.11.1 Flash-Aid 2.2.3 FoxyProxy Standard 4.1.3 InvisibleHand 3.8.28 LastPass 2.0.0 Memory Restart 1.11 Password ZHasher 1.1.7 Reddit Enhancement Suite 4.1.2 RequestPolicy 0.5.27 Test Pilot 1.2.2 Web Developer 1.2.2 Plugins ---------- Flash 11.1 r202 That security pref is set to false, on the Tabs preference page everything is selected but immediate switch to new tabs. I probably had a couple dozen tabs open in two tab groups, so I don't recollect anything specific on the URLs. Havent been able to intentionally reproduce it again yet.

https://crash-stats.mozilla.com/report/index/bp-a848ff22-605a-4f0b-9eca-dadae2130212 The comment on that crash is likely accurate for my case as well. I'm not sure what free memory was at, but Firefox was being slugging to respond, and I was closing tabs.

This doesn't quite make sense. mWindow is a weak reference, so we check if we get piwin before continuing: nsCOMPtr<nsPIDOMWindow> piwin = do_QueryReferent(mWindow); if (!piwin) return NS_OK; If piwin does exist, we try and get the docshell: nsIDocShell* docShell = piwin->GetDocShell(); MOZ_ASSERT(docShell); The docshell exists, as we can see in some (but not all) of the crash reports via the backtrace: https://crash-stats.mozilla.com/report/index/4eb98236-8549-4fad-81a0-733c22130212 nsDocShell::CreateContentViewer So where did the docshell go? Perhaps we should do a check for the docshell and return early if we can't find it, in addition to the check for piwin. if (!docshell) return NS_OK;

piwin->GetDocShell(); may return null, so we shouldn't assert. But MOZ_ASSERT is debug build only thing, so we crash later when calling docshell's methods. Some other OnLocationChange implementation may have done something odd which has closed the window.

(In reply to Olli Pettay [:smaug] from comment #16) > piwin->GetDocShell(); may return null, so we shouldn't assert. But > MOZ_ASSERT is debug build only thing, so we crash later when calling > docshell's methods. > Some other OnLocationChange implementation may have done something odd which > has closed the window. Per smaug, checking if the docshell exists as well, for cases where it ends up missing.

The way things are, the code just doesn't make sense. We *know* there is a DocShell that we're supposed to check with because it's on the call stack. IMO, the correct solution to these problems is to stop using mWindow to find the docshell. In nsDocShell::SetSecurityUI(), when we save mSecurityUI, we could also just call mSecurityUI->SetDocShell(this). That would avoid the whole mess, AFAICT.

(In reply to Brian Smith (:bsmith) from comment #18) > The way things are, the code just doesn't make sense. We *know* there is a > DocShell that we're supposed to check with because it's on the call stack. > > IMO, the correct solution to these problems is to stop using mWindow to find > the docshell. In nsDocShell::SetSecurityUI(), when we save mSecurityUI, we > could also just call mSecurityUI->SetDocShell(this). That would avoid the > whole mess, AFAICT. Brian, do you mean something like this?

Comment on attachment 713756 [details] Add mDocShell to nsSecureBrowserUIImpl Yes, except mDocShell would have to be a weak pointer, or we'd have to use the cycle collector mechanism.

Doesn't compile yet. Compile errors: http://www.pastebin.mozilla.org/2139919

It's #5 top browser crasher in 21.0a1.

Will run some mixed content tests on this to make sure they still work. Will also push to try.

Mixed content tests look good and pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=4992805484fd

Comment on attachment 714670 [details] [diff] [review] Add mDocShell to nsSecureBrowserUIImpl - v4 Review of attachment 714670 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/ipc/TabParent.h @@ +281,5 @@ > ScreenOrientation mOrientation; > float mDPI; > bool mShown; > bool mUpdatedDimensions; > + nsWeakPtr mDocShell; This isn't needed. TabParent claims to implement nsISecureBrowserUI but really it doesn't. In particular, look at: TabParent::GetState(uint32_t *aState) { NS_ENSURE_ARG(aState); NS_WARNING("SecurityState not valid here"); *aState = 0; return NS_OK; } NS_IMETHODIMP TabParent::GetTooltipText(nsAString & aTooltipText) { aTooltipText.Truncate(); return NS_OK; } SetDocShell() should be implemented similarly, with a similar NS_WARNING that it is doing nothing. (In fact, I wonder if TabParent::SetDocShell will ever get called at all?) ::: security/manager/boot/src/nsSecureBrowserUIImpl.cpp @@ +291,2 @@ > return NS_OK; > Question: It seems like only root DocShells should have an nsISecureBrowserUI associated with them. Is that the case? If that *is* the case, please we add an assertion that aDocShell is a root docshell. If that *isn't* the case, and mDocShell could be a non-root docshell, then don't we need to navigate from mDocShell to the root docshell? Because, I am pretty sure that only the root docshell maintains the proper security state now.

Updated the TabParent code. > > Question: It seems like only root DocShells should have an > nsISecureBrowserUI associated with them. Is that the case? > > If that *is* the case, please we add an assertion that aDocShell is a root > docshell. > > If that *isn't* the case, and mDocShell could be a non-root docshell, then > don't we need to navigate from mDocShell to the root docshell? Because, I am > pretty sure that only the root docshell maintains the proper security state > now. I will look into this.

> Question: It seems like only root DocShells should have an > nsISecureBrowserUI associated with them. Is that the case? We set the mixed active and mixed display flags on the rootDoc which can then be accessed by the root docShell in nsMixedContentBlocker.cpp, so it makes sense to use the root docShell here. I tried to find a case where we were in nsSecureBrowserUIImpl.cpp:MapInternaltoExternalState but are not the root docShell, and I can't seem to find one. I updated the patch that now adds an assertion that checks that the docshell is in fact the same type root doc shell. I have pushed it to try and also ran tests locally, but so far no failures: https://tbpl.mozilla.org/?tree=Try&rev=8acf65df6315 I'm not sure why this is true. onSecurityChange is called in the MixedContentBlocker on the nsIURI aRequestingContext. What if the https page is the iframed page (and hence the parent is not the https page) - http://people.mozilla.com/~tvyas/mixediframe2.html. Wouldn't we be calling onSecurityChange on the framed docshell then? bsmith, smaug - any ideas?

Potential patch; not sure if there is a way to have a non-root docShell call onSecurityChange and hit the assert.

Try shows that all debug builds failed for this patch.

Talked to Olli and read some code. * a DocShellTreeItem that is typeChrome cannot have a parent that is typeContent - http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsFrameLoader.cpp#705 * a DocShellTreeItem that is typeChrome will have a parent that is typeChrome. GetSameTypeRootTreeItem will return the root tree item that is typeChrome - https://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#2905. I'm not sure when we would create a securityUI for a chrome docshell that is not the root, but it may occur by an add-on. So I have added a special case for typeContent, which we know sets the Mixed Content state flags on the rootDocShell in nsMixedContentBlocker.cpp, and hence should check them on the rootDocShell.

Forgot to qfold with the previous patch; here is an update with the fold.

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=73ea9d39bd5d

Comment on attachment 716357 [details] [diff] [review] Add mDocShell to nsSecureBrowserUIImpl and get flags from the sameTypeRootDocShell v9 Review of attachment 716357 [details] [diff] [review]: ----------------------------------------------------------------- lgtm as long as try passes. You may need a review from a docshell peer.

Comment on attachment 716357 [details] [diff] [review] Add mDocShell to nsSecureBrowserUIImpl and get flags from the sameTypeRootDocShell v9 Requesting a second review from Olli the /docshell and /dom/interfaces changes.

Comment on attachment 716357 [details] [diff] [review] Add mDocShell to nsSecureBrowserUIImpl and get flags from the sameTypeRootDocShell v9 > interface nsITabParent : nsISupports > { >+ void setDocShell(in nsIDocShell docShell); > }; This looks really odd, and unused. Otherwise ok, but please remove this and explain the change to TabChild.cpp

s/TabChild/TabParent/

The patch breaks an about:addons test case that looks for insecure content in browser_discovery.js. I have been having trouble with this test and had filed bug 840388 to try and fix it.

This signature is spiking in Aurora 21 and is now #5 in the topcrash list.

The patch that fixes this is blocked on bug 840388.

(In reply to Tanvi Vyas [:tanvi] from comment #40) > The patch that fixes this is blocked on bug 840388. Tanvi, is that patch and the dependency planned to land on Aurora 21? If not, we need to find a different way to fix the crash, at worst a backout or deactivation of any patches that cause this.

Last week I was busy working on another project, but I am back to working on this. I have a patch in bug 840388 in review and will also update the patch attached here.

Updated patch. r? to smaug

Comment on attachment 720943 [details] [diff] [review] Add mDocShell to nsSecureBrowserUIImpl and get flags from the sameTypeRootDocShell v10 Carrying over r+ from bsmith.

Pushed to try, along with the patches in Bug 840388 and Bug 834836: https://tbpl.mozilla.org/?tree=Try&rev=3b9e02950972

We actually do need to update nsTabParent.cpp or we will get compile errors for not having implemented SetDocShell(). Updated patch.

Comment on attachment 720964 [details] [diff] [review] Add mDocShell to nsSecureBrowserUIImpl and get flags from the sameTypeRootDocShell v11 Carrying over bsmith's r+ again. Push to try: https://tbpl.mozilla.org/?tree=Try&rev=b9f4090c69dd

Comment on attachment 720964 [details] [diff] [review] Add mDocShell to nsSecureBrowserUIImpl and get flags from the sameTypeRootDocShell v11 > [scriptable, uuid(081e31e0-a144-11d3-8c7c-00609792278c)] update uuid

Updated uuid, carrying over r+ from bsmith and smaug. Now we need a good try (link is here: https://tbpl.mozilla.org/?tree=Try&rev=544583d935d6) and a review on bug 840388, and then we can push this.

Update: still waiting on bug 840388. I am meeting gavin and bz about 840388 later today to figure out what the right solution should be. Once we've figured that out, I'll implement a new patch.

The patch for bug 840388 is complete and r+'ed. The mochitests are done and I have just r?'ed them. Once we get an r+, we can land that bug and this bug. Hopefully we can uplift them both to Aurora so that the crash does affect FF 21 users when that moves to Beta or Stable. Thanks for your patience!

Bug 840388 is ready to land. I will land as soon as mozilla-inbound opens up again. Once it lands, we can land this and resolve the crash!

(In reply to Tanvi Vyas [:tanvi] from comment #52) > Bug 840388 is ready to land. I will land as soon as mozilla-inbound opens > up again. Once it lands, we can land this and resolve the crash! Thanks for the update here. We are really looking forward for this landing to resolve our top-crasher :) Once this stabilizes on m-c lets uplift at a very first chance we get to get this into aurora

Pushed to inbound! https://hg.mozilla.org/integration/mozilla-inbound/rev/ee27d1d8b302 Note if bug 840388 gets reverted from inbound or doesn't make it to central (for some reason; inbound has been pretty busted today), then this will need to be reverted too. (Without bug 840388, this patch will cause the browser_discovery.js test to fail.) I will keep an eye on the pushes. Once both bug patches make it to central and don't show signs of any issues, we should try and uplift both to aurora so FF 21 users don't experience crashes.

Whee, I managed to spot the dependency without having actually read the comment! Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/8136a81da037.

Phil, glad you caught the dependency! Update: Bug 840388 was rolled back because of an assertion failure caused by an unrelated bug (bug 855443). I have rewritten the tests to avoid the edge case in this bug and am waiting for review on the changes.

Pushed: http://hg.mozilla.org/integration/mozilla-inbound/rev/844ef68557d8

Bhavana, What is the process to uplift this (along with its dependency bug 840388) to aurora to resolve the aurora crashes?

(In reply to Tanvi Vyas [:tanvi] from comment #59) > What is the process to uplift this (along with its dependency bug 840388) to > aurora to resolve the aurora crashes? Nominate all needed patches for approval-mozilla-aurora (setting flags in attachment edit) and fill out the request form in the comment. Once you get them approved, land them on the releases/mozilla-aurora repo.

Comment on attachment 721954 [details] [diff] [review] Add mDocShell to nsSecureBrowserUIImpl and get flags from the sameTypeRootDocShell v12 [Approval Request Comment] Bug caused by (feature/regressing bug #): Bug 822367 and Bug 822371 User impact if declined: top crasher in Firefox 21 Testing completed (on m-c, etc.): landed on central. Risk to taking this patch (and alternatives if risky): Fixes a crash. Risk involved is the same as the risk of uplifting any code. Nothing specific to this patch that I know of. String or IDL/UUID changes made by this patch: UUID changes in /netwerk/base/public/nsISecureBrowserUI.idl.

(In reply to Tanvi Vyas [:tanvi] from comment #61) > Comment on attachment 721954 [details] [diff] [review] > Add mDocShell to nsSecureBrowserUIImpl and get flags from the > sameTypeRootDocShell v12 > > [Approval Request Comment] > Bug caused by (feature/regressing bug #): Bug 822367 and Bug 822371 > > User impact if declined: top crasher in Firefox 21 > > Testing completed (on m-c, etc.): landed on central. > > Risk to taking this patch (and alternatives if risky): Fixes a crash. Risk > involved is the same as the risk of uplifting any code. Nothing specific to > this patch that I know of. > > String or IDL/UUID changes made by this patch: UUID changes in > /netwerk/base/public/nsISecureBrowserUI.idl. Note that we cannot uplift this patch without also uplifting this patch in Bug 840388: https://bugzilla.mozilla.org/attachment.cgi?id=727060&action=edit

Comment on attachment 721954 [details] [diff] [review] Add mDocShell to nsSecureBrowserUIImpl and get flags from the sameTypeRootDocShell v12 Approving for uplift as this helps fix a top-crasher.

Pushed to aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/3274a156c130

No crashes with this signature since the date the patch was pushed to Aurora (29th of March).