Closed Bug 1067340 Opened 6 years ago Closed 6 years ago

content process crash in nsDOMWindowUtils::GetCurrentInnerWindowID

Categories

(Core :: DOM: Security, defect)

x86
Windows NT
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla35
Tracking Status
e10s ? ---

People

(Reporter: kairo, Assigned: ehsan)

References

Details

(Keywords: crash)

Crash Data

Attachments

(1 file, 2 obsolete files)

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.
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.
Summary: crash in nsDOMWindowUtils::GetCurrentInnerWindowID(unsigned __int64*) → content process crash in nsDOMWindowUtils::GetCurrentInnerWindowID
We're calling ExtractScheme on a null sIOService.
Blocks: 1061136
Flags: needinfo?(bobbyholley)
Sorry, wrong bug.
No longer blocks: 1061136
Flags: needinfo?(bobbyholley)
MOZ_RELEASE_ASSERT(nsContentUtils::IsCallerChrome()) at nsDOMWindowUtils::GetCurrentInnerWindowID
Flags: needinfo?(ehsan.akhgari)
Taking.
Assignee: nobody → ehsan.akhgari
tracking-e10s: --- → ?
Component: DOM: Security → DOM
Flags: needinfo?(ehsan.akhgari)
Attachment #8490067 - Flags: review?(bobbyholley)
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.
Attachment #8490067 - Flags: review?(bobbyholley) → review-
Attachment #8490067 - Attachment is obsolete: true
Component: DOM → DOM: Security
Attachment #8490442 - Flags: review?(sstamm)
See Also: → 1068389
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".
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 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.
Attachment #8490442 - Attachment is obsolete: true
Attachment #8490442 - Flags: review?(sstamm)
Attachment #8490900 - Flags: review?(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.
Attachment #8490900 - Flags: review?(sstamm) → review+
https://hg.mozilla.org/mozilla-central/rev/46859d329455
https://hg.mozilla.org/mozilla-central/rev/40efa924899c
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
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
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.