content process crash in nsDOMWindowUtils::GetCurrentInnerWindowID

VERIFIED FIXED in mozilla35

Status

()

--
critical
VERIFIED FIXED
4 years ago
4 years ago

People

(Reporter: kairo, Assigned: Ehsan)

Tracking

({crash})

Trunk
mozilla35
x86
Windows NT
crash
Points:
---

Firefox Tracking Flags

(e10s?)

Details

(crash signature)

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

4 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

4 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

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

Comment 6

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

Updated

4 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)

Comment 8

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

Updated

4 years ago
Attachment #8490067 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Component: DOM → DOM: Security
(Assignee)

Updated

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

Updated

4 years ago
See Also: → bug 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

4 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)

Comment 12

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

Updated

4 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
Last Resolved: 4 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.