1067340 - content process crash in nsDOMWindowUtils::GetCurrentInnerWindowID

Status: VERIFIED FIXED

Description

[Robert Kaiser]

This bug was filed from the Socorro interface and is 
report bp-b292686d-5758-4462-acfa-e103b2140915.
=============================================================

Top frames:
0 	xul.dll 	nsDOMWindowUtils::GetCurrentInnerWindowID(unsigned __int64*) 	dom/base/nsDOMWindowUtils.cpp
1 	xul.dll 	getInnerWindowID(nsIRequest*) 	content/base/src/nsCSPContext.cpp
2 	xul.dll 	nsCSPContext::SetRequestContext(nsIURI*, nsIURI*, nsIChannel*) 	content/base/src/nsCSPContext.cpp
3 	xul.dll 	nsDocument::InitCSP(nsIChannel*) 	content/base/src/nsDocument.cpp
4 	xul.dll 	nsDocument::StartDocumentLoad(char const*, nsIChannel*, nsILoadGroup*, nsISupports*, nsIStreamListener**, bool, nsIContentSink*) 	content/base/src/nsDocument.cpp
5 	xul.dll 	nsHTMLDocument::StartDocumentLoad(char const*, nsIChannel*, nsILoadGroup*, nsISupports*, nsIStreamListener**, bool, nsIContentSink*) 	content/html/document/src/nsHTMLDocument.cpp
6 	xul.dll 	nsContentDLF::CreateDocument(char const*, nsIChannel*, nsILoadGroup*, nsIDocShell*, nsID const&, nsIStreamListener**, nsIContentViewer**) 	layout/build/nsContentDLF.cpp
7 	xul.dll 	nsContentDLF::CreateInstance(char const*, nsIChannel*, nsILoadGroup*, char const*, nsIDocShell*, nsISupports*, nsIStreamListener**, nsIContentViewer**) 	layout/build/nsContentDLF.cpp
8 	xul.dll 	nsDocShell::NewContentViewerObj(char const*, nsIRequest*, nsILoadGroup*, nsIStreamListener**, nsIContentViewer**) 	docshell/base/nsDocShell.cpp
9 	xul.dll 	nsDocShell::CreateContentViewer(char const*, nsIRequest*, nsIStreamListener**) 	docshell/base/nsDocShell.cpp
10 	xul.dll 	nsDSURIContentListener::DoContent(char const*, bool, nsIRequest*, nsIStreamListener**, bool*) 	docshell/base/nsDSURIContentListener.cpp


This crash started rising on Nightly 35 with the 20140911064110 (so regression window is on 9/10).

For stats and more reports see https://crash-stats.mozilla.com/report/list?signature=nsDOMWindowUtils%3A%3AGetCurrentInnerWindowID%28unsigned%20__int64%2A%29

Given that it's in CSP code, I'm trying DOM:Security for now as a component.

Comment 1

[Robert Kaiser]

This is the first build after bug 1064885 so I guess any regression range I can get will point to this as the cause and this is an e10s regression.

Also note that those crashes are all in content processes.

Comment 2

[David Major [:dmajor]]

We're calling ExtractScheme on a null sIOService.

Comment 3

[David Major [:dmajor]]

Sorry, wrong bug.

Comment 4

[David Major [:dmajor]]

MOZ_RELEASE_ASSERT(nsContentUtils::IsCallerChrome()) at nsDOMWindowUtils::GetCurrentInnerWindowID

Comment 5

[:Ehsan Akhgari]

Taking.

Comment 6

[:Ehsan Akhgari]

Created attachment 8490067 [details] [diff] [review]
Revert the caller chrome assertion in nsDOMWindowUtils::GetOuterWindowID because the function now has non-JS callers; r=bholley

Comment 7

[Bobby Holley (:bholley)]

Comment on attachment 8490067 [details] [diff] [review]
Revert the caller chrome assertion in nsDOMWindowUtils::GetOuterWindowID because the function now has non-JS callers; r=bholley

IMO C++ callers shouldn't be using DOMWindowUtils. This caller (nsCSPContext) should just QI the nsIDOMWindow to nsPIDOMWindow, and invoke GetOuterWindow()->WindowID().

If there's some reason we can't do that, this fix is fine FWIW.

Comment 8

[:Ehsan Akhgari]

Created attachment 8490442 [details] [diff] [review]
Don't use nsIDOMWindowUtils in the CSP code, because it's not meant to be used from C++; r=geekboy

Comment 9

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 8490442 [details] [diff] [review]
Don't use nsIDOMWindowUtils in the CSP code, because it's not meant to be used from C++; r=geekboy

Review of attachment 8490442 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/base/src/nsCSPContext.cpp
@@ -529,5 @@
> -  if (NS_FAILED(rv)) {
> -    return 0;
> -  }
> -
> -  return id;

I think this changes the window ID that we're grabbing here for console messaging.  Looking at the implementation of nsIDOMWindowUtils::GetCurrentInnerWindowID(), I notice that the inner window is fetched from the nsPIDOMWindow (not the outer window).  

While I think this is probably going to work (based on a scan of the implementation of nsIScriptError::InitWithWindowID), it's referred to as inner window everywhere and that seems wrong to me if we're passing around an outer window ID.

Ehsan, you probably know more about how this works than me, but I'd like the names of the variables and what they contain to match, at least in the CSP code.  Please either grab the inner window or replace all the references to "innerWindowID" in nsCSPContext.cpp/h and nsCSPUtils.cpp/h with either "outerWindowID" or the more generic "windowID".

Comment 10

[Sid Stamm [:geekboy or :sstamm]]

Of course the context included by splinter is not helpful in comment 9.  Here's the relevant context:

> - nsCOMPtr<nsIDOMWindowUtils> du = do_GetInterface(window);
> - rv = du->GetCurrentInnerWindowID(&id);
> - if (NS_FAILED(rv)) {		
> -   return 0;		
> - }		
> - return id;
 		
> + nsCOMPtr<nsPIDOMWindow> pwindow = do_QueryInterface(window);
> + return pwindow->GetOuterWindow()->WindowID();

Comment 11

[:Ehsan Akhgari]

Comment on attachment 8490442 [details] [diff] [review]
Don't use nsIDOMWindowUtils in the CSP code, because it's not meant to be used from C++; r=geekboy

Sure, let's use the inner window.

Comment 12

[:Ehsan Akhgari]

Created attachment 8490900 [details] [diff] [review]
Don't use nsIDOMWindowUtils in the CSP code, because it's not meant to be used from C++; r=geekboy

Comment 13

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 8490900 [details] [diff] [review]
Don't use nsIDOMWindowUtils in the CSP code, because it's not meant to be used from C++; r=geekboy

Review of attachment 8490900 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/46859d329455
https://hg.mozilla.org/mozilla-central/rev/40efa924899c

Comment 15

[Florin Mezei, QA (:FlorinMezei)]

Socorro [1] shows only 3 crashes over the past 4 weeks, and all of them are on Firefox 33 Beta.

[1] - https://crash-stats.mozilla.com/report/list?product=Firefox&range_unit=days&range_value=28&signature=nsDOMWindowUtils%3A%3AGetCurrentInnerWindowID%28unsigned+__int64%2A%29