837075 - crash in nsDocShell::GetAllowMixedContentAndConnectionData @ nsINode::NodePrincipal

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, 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 	nsINode::NodePrincipal() More Reports Search
UUID	accd880e-bb9e-4a01-869b-5b00c2130201
Date Processed	2013-02-01 10:44:54
Uptime	4
Last Crash	42 seconds before submission
Install Age	18.4 hours since version was first installed.
Install Time	2013-01-31 16:19:21
Product	Firefox
Version	21.0a1
Build ID	20130131031009
Release Channel	nightly
OS	Windows NT
OS Version	5.1.2600 Service Pack 3
Build Architecture	x86
Build Architecture Info	GenuineIntel family 6 model 23 stepping 6
Crash Reason	EXCEPTION_ACCESS_VIOLATION_READ
Crash Address	0xc
App Notes 	
AdapterVendorID: 0x10de, AdapterDeviceID: 0x0e22, AdapterSubsysID: 00000000, AdapterDriverVersion: 6.14.13.681
D3D10 Layers? D3D10 Layers- 
Processor Notes 	sp-processor08.phx1.mozilla.com_14844:2008
EMCheckCompatibility	False
Adapter Vendor ID	0x10de
Adapter Device ID	0x0e22
Total Virtual Memory	2147352576
Available Virtual Memory	1925124096
System Memory Use Percentage	36
Available Page File	4249829376
Available Physical Memory	2123603968

Frame 	Module 	Signature 	Source
0 	xul.dll 	nsINode::NodePrincipal 	obj-firefox/dist/include/nsINode.h:744
1 	xul.dll 	nsDocShell::GetAllowMixedContentAndConnectionData 	docshell/base/nsDocShell.cpp:5389
2 	xul.dll 	nsHTMLDocument::Open 	content/html/document/src/nsHTMLDocument.cpp:1629
3 	xul.dll 	nsHTMLDocument::Open 	content/html/document/src/nsHTMLDocument.cpp:1410
4 	xul.dll 	nsHTMLDocument::WriteCommon 	content/html/document/src/nsHTMLDocument.cpp:1890
5 	xul.dll 	nsHTMLDocument::WriteCommon 	content/html/document/src/nsHTMLDocument.cpp:1824
6 	xul.dll 	mozilla::dom::HTMLDocumentBinding::write 	obj-firefox/dom/bindings/HTMLDocumentBinding.cpp:648
7 	xul.dll 	mozilla::dom::HTMLDocumentBinding::genericMethod 	obj-firefox/dom/bindings/HTMLDocumentBinding.cpp:1536
8 	mozjs.dll 	js::InvokeKernel 	js/src/jsinterp.cpp:390
9 	mozjs.dll 	js::Invoke 	js/src/jsinterp.cpp:437
10 	mozjs.dll 	js::CrossCompartmentWrapper::call 	js/src/jswrapper.cpp:631
11 	mozjs.dll 	proxy_Call 	js/src/jsproxy.cpp:3015
12 	mozjs.dll 	js::InvokeKernel 	js/src/jsinterp.cpp:383
13 	mozjs.dll 	js::Interpret 	js/src/jsinterp.cpp:2376
...

More reports at:
https://crash-stats.mozilla.com/report/list?signature=nsINode%3A%3ANodePrincipal%28%29

Comment 1

[Boris Zbarsky [:bzbarsky]]

The relevant code is:

5386   nsCOMPtr<nsIDocument> rootDoc = do_GetInterface(sameTypeRoot);
5387   NS_ASSERTION(rootDoc, "No root document from document shell root tree item.");
5388 
5389   nsCOMPtr<nsIPrincipal> rootPrincipal = rootDoc->NodePrincipal();
5390   NS_ASSERTION(rootPrincipal, "No root principal from root document");

and the crash is on line 5389 with a null-deref.  Presumably either sameTypeRoot is ending up null or the do_GetInterface is returning null...

Comment 2

[Tanvi Vyas[:tanvi]]

This is occurring after a document.open().  Trying to reproduce.

Comment 3

[Tanvi Vyas[:tanvi]]

Attachment: Check if rootDoc exists before checking if the page is https and if the user has allowed mixed content

This should fix the crash, but I think we still need a test case to show
1) this patch actually fixes the issue
2) what types of situations are there were we don't have a rootDoc but have a document.open().  I tried some of the developer tools, but haven't had any luck so far.

Comment 4

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

Comment on attachment 709209 [details] [diff] [review]
Check if rootDoc exists before checking if the page is https and if the user has allowed mixed content

(nsCOMPtr<nsIDocShell> rootDocShell = do_GetInterface(sameTypeRoot); should use
QueryInterface, not GetInterface. Both should work in this case, but QI is a bit faster)

I wonder if the crash happens when we're about to destroy the sameTypeRoot
(mIsBeingDestroyed == true), yet some script is executing document.write for a
subdocument.

I think we should change the default for aRootHasSecureConnection.
Make it true, and if principal is chrome or uri is null or non-https, then change it to false.

Comment 5

[Tanvi Vyas[:tanvi]]

Attachment: Check if rootDoc exists before checking if the page is https and if the user has allowed mixed content v2

(In reply to Olli Pettay [:smaug] from comment #4)
> Comment on attachment 709209 [details] [diff] [review]
> Check if rootDoc exists before checking if the page is https and if the user
> has allowed mixed content
> 
> (nsCOMPtr<nsIDocShell> rootDocShell = do_GetInterface(sameTypeRoot); should
> use
> QueryInterface, not GetInterface. Both should work in this case, but QI is a
> bit faster)
> 
Are you sure?  Both nsCOMPtr<nsIDocument> rootDoc(do_QueryInterface(sameTypeRoot)); and nsCOMPtr<nsIDocument> rootDoc = do_QueryInterface(sameTypeRoot); give me a null rootdoc.  Am I missing something?

> I wonder if the crash happens when we're about to destroy the sameTypeRoot
> (mIsBeingDestroyed == true), yet some script is executing document.write for
> a
> subdocument.
> 

This is a windows only crash.  Going to see if I can borrow a windows laptop from Desktop today, but so far I have had no luck reproducing this on Mac, and dveditz had no luck reproducing it on Windows with some test cases I wrote for this bug.

> I think we should change the default for aRootHasSecureConnection.
> Make it true, and if principal is chrome or uri is null or non-https, then
> change it to false.

I did this.

Comment 6

[Boris Zbarsky [:bzbarsky]]

> Both nsCOMPtr<nsIDocument>

Olli was talking about nsIDocShell, not nsIDocument....

Comment 7

[Tanvi Vyas[:tanvi]]

Attachment: Check if rootDoc exists before checking if the page is https and if the user has allowed mixed content v3

(In reply to Boris Zbarsky (:bz) from comment #6)
> > Both nsCOMPtr<nsIDocument>
> 
> Olli was talking about nsIDocShell, not nsIDocument....

Thanks BZ.  Here is an updated patch.

Comment 8

[Scoobidiver (away)]

It's #10 top browser crasher over the last three days.

Comment 9

[Tanvi Vyas[:tanvi]]

Got a Windows machine from IT with Windows 7 on it and I am still not able to reproduce the crash.

Version 6.1 Build 7600

Virgil, do you have some time to help and see if you can reproduce this crash?

Comment 10

[Tanvi Vyas[:tanvi]]

I tried all the urls I could find from the crash-stats and tried session restore (since many reports have a short uptime).  No luck.  I will wait for Olli's review comments.

Comment 11

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

Comment on attachment 709983 [details] [diff] [review]
Check if rootDoc exists before checking if the page is https and if the user has allowed mixed content v3


>+  if (rootDoc) {
>+    nsCOMPtr<nsIPrincipal> rootPrincipal = rootDoc->NodePrincipal();
>+    NS_ASSERTION(rootPrincipal, "No root principal from root document");
You could drop this assertion. NodePrincipal() doesn't start with Get* and in general
such methods in DOM code can't fail.


>+    // For things with system principal (e.g. scratchpad) there is no uri
>+    // aRootHasSecureConnection should be false.
>+    if (nsContentUtils::IsSystemPrincipal(rootPrincipal)) {
>+      *aRootHasSecureConnection = false;
>+    } else {
>+      nsCOMPtr<nsIURI> rootUri;
>+      rootPrincipal->GetURI(getter_AddRefs(rootUri));
>+      NS_ASSERTION(rootUri, "No root uri from root principal");
>+      nsresult rv = rootUri->SchemeIs("https", aRootHasSecureConnection);
>+      NS_ENSURE_SUCCESS(rv, rv);
Could you make this 
nsCOMPtr<nsIURI> rootUri;
if (nsContentUtils::IsSystemPrincipal(rootPrincipal) ||
    NS_FAILED(rootPrincipal->GetURI(getter_AddRefs(rootUri))) || !rootURI ||
    NS_FAILED(rootUri->SchemeIs("https", aRootHasSecureConnection))) {
  *aRootHasSecureConnection = false;
}

Comment 12

[Tanvi Vyas[:tanvi]]

Attachment: Check if rootDoc exists before checking if the page is https and if the user has allowed mixed content v4

Addressed Olli's review comments.  Carrying over r+.

Here is a push to try:https://tbpl.mozilla.org/?tree=Try&rev=a8694f84f167

Will land after I get the results.

Comment 13

[Virgil Dicu [:virgil] [QA]]

(In reply to Tanvi Vyas [:tanvi] from comment #9)
> Virgil, do you have some time to help and see if you can reproduce this
> crash?

Mozilla/5.0 (Windows NT 6.1; rv:21.0) Gecko/20130205 Firefox/21.0

Tried on Windows 7, but couldn't reproduce unfortunately - there's not much data at the moment to try and reproduce. Turned the prefs on/off, navigated to random mixed content sites, remember password/log out/on (e.g. quality.mozilla.org), installed security related add-ons. Session restore, several browser restarts, disabled/enabled protection through doorhanger.

Comment 14

[Tanvi Vyas[:tanvi]]

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

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1a489ff14285

Comment 16

[Virgil Dicu [:virgil] [QA]]

https://crash-stats.mozilla.com/report/list?query_search=signature&query_type=contains&reason_type=contains&range_value=2&range_unit=weeks&hang_type=any&process_type=any&signature=nsINode%3A%3ANodePrincipal%28%29

No crashes with this signature in the last two weeks - since the patch landed. It's ok to call it verified.