836951 - crash in nsSecureBrowserUIImpl::MapInternalToExternalState

Status: VERIFIED FIXED

(Core :: Security, defect)

Description

[Scoobidiver (away)]

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

Comment 1

[Tanvi Vyas[:tanvi]]

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)

Comment 2

[Tanvi Vyas[:tanvi]]

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

Comment 3

[Marcia Knous [:marcia]]

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

Comment 4

[Tanvi Vyas[:tanvi]]

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

Still looking into this.

Comment 5

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

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.

Comment 6

[Brent Garber]

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.

Comment 7

[Daniel Veditz [:dveditz]]

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

Comment 8

[Tanvi Vyas[:tanvi]]

Attachment: Null check for piwin before requesting docshell v1

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

Comment 9

[Tanvi Vyas[:tanvi]]

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

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d2353a795fb7

Comment 11

[Scoobidiver (away)]

There are still crashes.

Comment 12

[Virgil Dicu [:virgil] [QA]]

(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

Comment 13

[Brent Garber]

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.

Comment 14

[Brent Garber]

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.

Comment 15

[Tanvi Vyas[:tanvi]]

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;

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 17

[Tanvi Vyas[:tanvi]]

Attachment: Null check for docshell v1

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

Comment 18

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

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.

Comment 19

[Tanvi Vyas[:tanvi]]

Attachment: Add mDocShell to nsSecureBrowserUIImpl

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

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

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.

Comment 21

[Tanvi Vyas[:tanvi]]

Attachment: Add mDocShell to nsSecureBrowserUIImpl - WIP v2

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

Comment 22

[Scoobidiver (away)]

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

Comment 23

[Tanvi Vyas[:tanvi]]

Attachment: Add mDocShell to nsSecureBrowserUIImpl - WIP v3

Comment 24

[Tanvi Vyas[:tanvi]]

Attachment: Add mDocShell to nsSecureBrowserUIImpl - v4

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

Comment 25

[Tanvi Vyas[:tanvi]]

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

Comment 26

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

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.

Comment 27

[Tanvi Vyas[:tanvi]]

Attachment: Add mDocShell to nsSecureBrowserUIImpl - v5

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.

Comment 28

[Tanvi Vyas[:tanvi]]

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

Comment 29

[Tanvi Vyas[:tanvi]]

Attachment: Add mDocShell to nsSecureBrowserUIImpl and assert that it is the root doc shell - v6

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

Comment 30

[Tanvi Vyas[:tanvi]]

Try shows that all debug builds failed for this patch.

Comment 31

[Tanvi Vyas[:tanvi]]

Attachment: Add mDocShell to nsSecureBrowserUIImpl and get flags from the sameTypeRootDocShell v7

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.

Comment 32

[Tanvi Vyas[:tanvi]]

Attachment: Add mDocShell to nsSecureBrowserUIImpl and get flags from the sameTypeRootDocShell v8

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

Comment 33

[Tanvi Vyas[:tanvi]]

Attachment: Add mDocShell to nsSecureBrowserUIImpl and get flags from the sameTypeRootDocShell v9

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

Comment 34

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

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 35

[Tanvi Vyas[:tanvi]]

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 36

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 37

[Olli Pettay [:smaug][[e-mail]]]

s/TabChild/TabParent/

Comment 38

[Tanvi Vyas[:tanvi]]

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.

Comment 39

[Robert Kaiser]

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

Comment 40

[Tanvi Vyas[:tanvi]]

The patch that fixes this is blocked on bug 840388.

Comment 41

[Robert Kaiser]

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

Comment 42

[Tanvi Vyas[:tanvi]]

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.

Comment 43

[Tanvi Vyas[:tanvi]]

Attachment: Add mDocShell to nsSecureBrowserUIImpl and get flags from the sameTypeRootDocShell v10

Updated patch.  r? to smaug

Comment 44

[Tanvi Vyas[:tanvi]]

Comment on attachment 720943 [details] [diff] [review]
Add mDocShell to nsSecureBrowserUIImpl and get flags from the sameTypeRootDocShell v10

Carrying over r+ from bsmith.

Comment 45

[Tanvi Vyas[:tanvi]]

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

Comment 46

[Tanvi Vyas[:tanvi]]

Attachment: Add mDocShell to nsSecureBrowserUIImpl and get flags from the sameTypeRootDocShell v11

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

Comment 47

[Tanvi Vyas[:tanvi]]

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 48

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 49

[Tanvi Vyas[:tanvi]]

Attachment: Add mDocShell to nsSecureBrowserUIImpl and get flags from the sameTypeRootDocShell v12

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.

Comment 50

[Tanvi Vyas[:tanvi]]

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.

Comment 51

[Tanvi Vyas[:tanvi]]

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!

Comment 52

[Tanvi Vyas[:tanvi]]

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!

Comment 53

[bhavana bajaj [:bajaj]]

(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

Comment 54

[Tanvi Vyas[:tanvi]]

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.

Comment 55

[Phil Ringnalda (:philor)]

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.

Comment 56

[Tanvi Vyas[:tanvi]]

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.

Comment 57

[Tanvi Vyas[:tanvi]]

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

Comment 58

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/844ef68557d8

Comment 59

[Tanvi Vyas[:tanvi]]

Bhavana,

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

Comment 60

[Robert Kaiser]

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

[Tanvi Vyas[:tanvi]]

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.

Comment 62

[Tanvi Vyas[:tanvi]]

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

[bhavana bajaj [:bajaj]]

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.

Comment 64

[Tanvi Vyas[:tanvi]]

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

Comment 65

[Virgil Dicu [:virgil] [QA]]

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