Closed Bug 996280 Opened 6 years ago Closed 6 years ago

crash in nsDocShell::IsSandboxedFrom(nsIDocShell*)

Categories

(Core :: DOM: Navigation, defect, critical)

29 Branch
x86
Windows NT
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla32
Tracking Status
firefox29 --- wontfix
firefox30 --- verified
firefox31 --- verified
firefox32 --- verified
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed

People

(Reporter: jbecerra, Assigned: bobowen)

Details

(Keywords: crash)

Crash Data

Attachments

(1 file, 1 obsolete file)

This bug was filed from the Socorro interface and is 
report bp-740ba6dd-a2e9-488c-ae57-b1d712140414.
=============================================================

Currently on the top 50 crashers for Firefox 29, but it has been going up recently. It's mostly on Windows 7 and XP installations. Nothing jumps at me when looking at the correlations. There are a handful of comments in the reports, one points to just visiting a site that takes a long time to load and then the application crashes, and another which says that the application crashed while selecting an item from a drop down menu. 

Signature 	nsDocShell::IsSandboxedFrom(nsIDocShell*) More Reports Search
UUID 	740ba6dd-a2e9-488c-ae57-b1d712140414
Date Processed	2014-04-14 09:38:05.989132
Uptime	221
Install Age 	182417 since version was first installed.
Install Time 	2014-04-12 06:57:05
Product 	Firefox
Version 	29.0
Build ID 	20140410150427
Release Channel 	beta
OS 	Windows NT
OS Version 	6.1.7601 Service Pack 1
Build Architecture 	x86
Build Architecture Info 	GenuineIntel family 6 model 37 stepping 5 | 4
Crash Reason 	EXCEPTION_ACCESS_VIOLATION_READ
Crash Address 	0x0
Email Address 	
URL 	about:blank - Super Sensitive! Don't mess around!
Exploitability 	none
User Comments 	
App Notes 	

AdapterVendorID: 0x8086, AdapterDeviceID: 0x0046, AdapterSubsysID: 04471028, AdapterDriverVersion: 8.15.10.2559
D2D? D2D+ DWrite? DWrite+ D3D10 Layers? D3D10 Layers+ 

Processor Notes 	sp-processor08_phx1_mozilla_com.17679:2012; HybridCrashProcessor; non-integer value of "SecondsSinceLastCrash"
EMCheckCompatibility
True

Winsock LSP 	

MSAFD Tcpip [TCP/IP] : 2 : 1 : %SystemRoot%\system32\mswsock.dll 
 MSAFD Tcpip [UDP/IP] : 2 : 2 :  
 MSAFD Tcpip [RAW/IP] : 2 : 3 : %SystemRoot%\system32\mswsock.dll 
 MSAFD Tcpip [TCP/IPv6] : 2 : 1 :  
 MSAFD Tcpip [UDP/IPv6] : 2 : 2 : %SystemRoot%\system32\mswsock.dll 
 MSAFD Tcpip [RAW/IPv6] : 2 : 3 :  
 RSVP TCPv6 Service Provider : 2 : 1 : %SystemRoot%\system32\mswsock.dll 
 RSVP TCP Service Provider : 2 : 1 :  
 RSVP UDPv6 Service Provider : 2 : 2 : %SystemRoot%\system32\mswsock.dll 
 RSVP UDP Service Provider : 2 : 2 :  
 VMCI sockets DGRAM : 0 : 2 : %windir%\system32\vsocklib.dll 
 VMCI sockets STREAM : 0 : 1 : 

Adapter Vendor ID 	
0x8086

Adapter Device ID 	
0x0046

Total Virtual Memory 	
4294836224

Available Virtual Memory 	
3570188288

System Memory Use Percentage 	
44

Available Page File 	
6149709824

Available Physical Memory 	
2269863936

Frame 	Module 	Signature 	Source
0 	xul.dll 	nsDocShell::IsSandboxedFrom(nsIDocShell *) 	docshell/base/nsDocShell.cpp
1 	xul.dll 	nsDocShell::InternalLoad(nsIURI *,nsIURI *,nsISupports *,unsigned int,wchar_t const *,char const *,nsAString_internal const &,nsIInputStream *,nsIInputStream *,unsigned int,nsISHEntry *,bool,nsAString_internal const &,nsIDocShell *,nsIDocShell * *,nsIRequest * *) 	docshell/base/nsDocShell.cpp
2 	xul.dll 	nsCOMPtr_base::assign_from_qi(nsQueryInterface,nsID const &) 	xpcom/glue/nsCOMPtr.cpp
3 	xul.dll 	nsTHashtable<nsCStringHashKey>::s_MatchEntry(PLDHashTable *,PLDHashEntryHdr const *,void const *) 	obj-firefox/dist/include/nsTHashtable.h
4 	xul.dll 	xul.dll@0x1360f2c

More reports at: https://crash-stats.mozilla.com/report/list?product=Firefox&signature=nsDocShell%3A%3AIsSandboxedFrom%28nsIDocShell%2A%29
This has gone up in the list of top crashers nearing the top 20.
Null mContentViewer in nsDocShell::IsSandboxedFrom. Bob are you the right person to look at this?
Flags: needinfo?(bobowencode)
(In reply to David Major [:dmajor] (UTC+12) from comment #2)
> Null mContentViewer in nsDocShell::IsSandboxedFrom. Bob are you the right
> person to look at this?

It's definitely my patch for bug 785310 that introduced this. :-(

nsDocShell::IsSandboxedFrom used to be static and the accessing docshell's document was retrieved using do_GetInterface on the nsIDocShellTreeItem.
This calls EnsureContentViewer before getting the document.

So, the safest thing here seems to be to revert to the same.
Although, it does seem slightly wrong, as if EnsureContentViewer creates a new document the sandbox flags could in theory be different from the ones on the document that actually caused the navigation attempt.
But at least this would pick up the source docshell's current sandbox flags.
Also, should we be causing a potential state change on the source docshell at this point?

The other alternative is a null check on mContentViewer, but if it is do we just assume that there is no sandbox?
We could use the docshell's flags, which would have the same effect as the above or we could just block the load.
None of this is in the spec, but I think the spec is assuming that the source browsing context (docshell) should always have an active document (i.e. the one that caused the navigation) when these checks are done.

IsSandboxedFrom used to be called in FindItemWithName, so before we had transferred the load to the target docshell, which may mean that mContentViewer was never null.
Now that we do the checks in the InternalLoad of the target and further down the function after a potential async call, I suppose there is more chance that the source doc shell could be in a state where mContentViewer is null.

Ideally, as I've mentioned before, it would be good to get rid of all of these confusing recursive calls to InternalLoad, where the code at the top of the function can get run multiple times and has to keep checking if it is in "find navigation target" or "actual load" mode.
That's a change that's going to take a lot longer than this quick fix though. :-)

Try push with the do_GetInterface:
https://tbpl.mozilla.org/?tree=Try&rev=abe7cd987e63
Assignee: nobody → bobowencode
Attachment #8417923 - Flags: review?(bobbyholley)
Flags: needinfo?(bobowencode)
Comment on attachment 8417923 [details] [diff] [review]
Bug 996280: Use do_GetInterface to get document in nsDocShell::IsSanboxedFrom, to prevent crash.

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

I think smaug probably has a better opinion of which way we want to go here.
Attachment #8417923 - Flags: review?(bobbyholley) → review?(bugs)
(In reply to Bob Owen (:bobowen) from comment #3)
> So, the safest thing here seems to be to revert to the same.
> Although, it does seem slightly wrong, as if EnsureContentViewer creates a
> new document the sandbox flags could in theory be different from the ones on
> the document that actually caused the navigation attempt.
But we're not dealing the document which caused the navigation attempt here.
So it is not clear to me what you're worrying here.


> But at least this would pick up the source docshell's current sandbox flags.
Indeed.

> Also, should we be causing a potential state change on the source docshell
> at this point?
hmm, which state change?
Comment on attachment 8417923 [details] [diff] [review]
Bug 996280: Use do_GetInterface to get document in nsDocShell::IsSanboxedFrom, to prevent crash.

What would it look like if we explicitly checked the sandbox flags here
the same way a new document gets them from parent/opener?
Attachment #8417923 - Flags: review?(bugs) → feedback?(bobowencode)
Comment on attachment 8417923 [details] [diff] [review]
Bug 996280: Use do_GetInterface to get document in nsDocShell::IsSanboxedFrom, to prevent crash.

Could you check that.
(it would be nice to avoid creating about:blank here)
If it gets complicated, re-ask review.
Attachment #8417923 - Flags: feedback?(bobowencode)
Flags: needinfo?(bobowencode)
(In reply to Olli Pettay [:smaug] from comment #5)
> (In reply to Bob Owen (:bobowen) from comment #3)
> > So, the safest thing here seems to be to revert to the same.
> > Although, it does seem slightly wrong, as if EnsureContentViewer creates a
> > new document the sandbox flags could in theory be different from the ones on
> > the document that actually caused the navigation attempt.
> But we're not dealing the document which caused the navigation attempt here.
> So it is not clear to me what you're worrying here.

But we need to get the source docshell's active document's sandbox flags, to see if it is allowed to navigate us.
If the mContentViewer is null, then that document is no longer around.
  
> > Also, should we be causing a potential state change on the source docshell
> > at this point?
> hmm, which state change?

EnsureContentViewer could create a new ContentViewer and Document, I think.

(In reply to Olli Pettay [:smaug] from comment #7)
> Comment on attachment 8417923 [details] [diff] [review]
> Bug 996280: Use do_GetInterface to get document in
> nsDocShell::IsSanboxedFrom, to prevent crash.
> 
> Could you check that.
> (it would be nice to avoid creating about:blank here)
> If it gets complicated, re-ask review.

So, my alternative from comment 3?
Status: NEW → ASSIGNED
Flags: needinfo?(bobowencode)
Ah, yes, I misunderstood the alternative. I thought it would be more like just a null check.
But yeah, don't ensure contentviewer, but just check the flags, if possible.
(In reply to Olli Pettay [:smaug] from comment #6)
> Comment on attachment 8417923 [details] [diff] [review]
> Bug 996280: Use do_GetInterface to get document in
> nsDocShell::IsSanboxedFrom, to prevent crash.
> 
> What would it look like if we explicitly checked the sandbox flags here
> the same way a new document gets them from parent/opener?

Thanks Olli, yes I think this is probably the best quick fix.
Attachment #8418220 - Flags: review?(bugs)
Attachment #8417923 - Attachment is obsolete: true
Attachment #8418220 - Flags: review?(bugs) → review+
Thanks smaug, try push for this patch:
https://tbpl.mozilla.org/?tree=Try&rev=965e9079d211
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/d1202c98de44
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
David, do you think this is something that should be considered for uplift?
Flags: needinfo?(dmajor)
(In reply to Bob Owen (:bobowen) from comment #14)
> David, do you think this is something that should be considered for uplift?

I'm not familiar with this code so I can only offer general advice. It depends on risk. Is it safe to use the current flags if mContentViewer is null? Sometimes it's better to crash than do something unsafe. If that's not a concern in this case, then I would be in favor of an uplift. This is currently the #16 crash on 29.
Flags: needinfo?(dmajor)
Comment on attachment 8418220 [details] [diff] [review]
Use the docshell's sandbox flags if we can't get the active document in IsSandboxedFrom() v2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): caused by fix for bug 785310, the check is now done slightly differently and later in the navigation algorithm, which means the source docshell may no longer have a content viewer.
If this could have happened with the old code then, because it used do_GetInterface, which calls EnsureContentViewer, the document returned would have the source docshell's sandbox flags copied to it.
So, this fix should give the same effect, but without changing the source docshell.

User impact if declined: they will still experience this crash currently #16 for firefox29.

Testing completed (on m-c, etc.): Unable to reproduce the crash, so difficult to test this directly.

Risk to taking this patch (and alternatives if risky): Medium - this is a sensitive area of code, however the fix is pretty straight forward.
Checked with reviewer (smaug) on IRC and he thinks that it should be safe to uplift.

String or IDL/UUID changes made by this patch: None.
Attachment #8418220 - Flags: approval-mozilla-beta?
Attachment #8418220 - Flags: approval-mozilla-aurora?
Attachment #8418220 - Flags: approval-mozilla-beta?
Attachment #8418220 - Flags: approval-mozilla-beta+
Attachment #8418220 - Flags: approval-mozilla-aurora?
Attachment #8418220 - Flags: approval-mozilla-aurora+
No crashes on 32.0a1 in the crashstats after 2014-05-08.
1. Mozilla/5.0 (Windows NT 5.2; rv:30.0) Gecko/20100101 Firefox/30.0
   Mozilla/5.0 (Windows NT 6.1; rv:30.0) Gecko/20100101 Firefox/30.0
     Build ID: 20140522105902
2.Mozilla/5.0 (Windows NT 5.2; rv:31.0) Gecko/20100101 Firefox/31.0
  Mozilla/5.0 (Windows NT 6.1; rv:31.0) Gecko/20100101 Firefox/31.0		
     Build ID: 20140526004004

Unable to reproduce this issue on (1) Firefox 30 beta 7 and (2) latest Aurora builds.

There are no crashes for the latest Beta versions in Socorro (beginning with Fx 30b5).

For Aurora 31.0a2 there still are 3 crashes in Socorro for the latest week. Not sure how worried we should be about this. What do you think?
Flags: needinfo?(bobowencode)
QA Contact: cornel.ionce
(In reply to Cornel Ionce [QA] from comment #20)

Thanks for checking all of this.

> For Aurora 31.0a2 there still are 3 crashes in Socorro for the latest week.
> Not sure how worried we should be about this. What do you think?

This is the first time I've dealt with the Aurora and Beta build versions or Socorro, so I'm not totally clear how they tie up.

However, the Build column on the Reports tab looks like a time-stamp and if I sort by that in descending order, we don't appear to have had any crashes in builds after 15/5/2014, which is the day the patch was uplifted.
So, I think it's looks OK and these are just from old versions of Aurora.
Flags: needinfo?(bobowencode)
No crashes on 31 beta in the crashstats.
Status: RESOLVED → VERIFIED
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.