content process crash in nsDOMWindowUtils::GetCurrentInnerWindowID

VERIFIED FIXED in mozilla35

Status

()

defect
--
critical
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: kairo, Assigned: Ehsan)

Tracking

({crash})

Trunk
mozilla35
x86
Windows NT
Points:
---

Firefox Tracking Flags

(e10s?)

Details

(crash signature)

Attachments

(1 attachment, 2 obsolete attachments)

Reporter

Description

5 years ago
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.
Reporter

Comment 1

5 years ago
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)
Assignee

Comment 5

5 years ago
Taking.
Assignee: nobody → ehsan.akhgari
tracking-e10s: --- → ?
Component: DOM: Security → DOM
Flags: needinfo?(ehsan.akhgari)
Assignee

Updated

5 years ago
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-
Assignee

Updated

5 years ago
Attachment #8490067 - Attachment is obsolete: true
Assignee

Updated

5 years ago
Component: DOM → DOM: Security
Assignee

Updated

5 years ago
Attachment #8490442 - Flags: review?(sstamm)
Assignee

Updated

5 years ago
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();
Assignee

Comment 11

5 years ago
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)
Assignee

Updated

5 years ago
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: 5 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.