996069 - crash in CreateNativeGlobalForInner starting on 2014-04-11

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[u279076]

This bug was filed from the Socorro interface and is 
report bp-ff4242d5-e8f9-42f9-9b78-afbf22140411.
=============================================================
0 	xul.dll 	CreateNativeGlobalForInner 	dom/base/nsGlobalWindow.cpp
1 	xul.dll 	nsGlobalWindow::SetNewDocument(nsIDocument *,nsISupports *,bool) 	dom/base/nsGlobalWindow.cpp
2 	xul.dll 	nsDocumentViewer::InitInternal(nsIWidget *,nsISupports *,nsIntRect const &,bool,bool,bool) 	layout/base/nsDocumentViewer.cpp
3 	xul.dll 	nsDocumentViewer::Init(nsIWidget *,nsIntRect const &) 	layout/base/nsDocumentViewer.cpp
4 	xul.dll 	nsDocShell::SetupNewViewer(nsIContentViewer *) 	docshell/base/nsDocShell.cpp
5 	xul.dll 	nsDocShell::Embed(nsIContentViewer *,char const *,nsISupports *) 	docshell/base/nsDocShell.cpp
6 	xul.dll 	nsDocShell::CreateAboutBlankContentViewer(nsIPrincipal *,nsIURI *,bool) 	docshell/base/nsDocShell.cpp
7 	xul.dll 	nsDocShell::CreateAboutBlankContentViewer(nsIPrincipal *) 	docshell/base/nsDocShell.cpp
8 	xul.dll 	nsGlobalWindow::SetInitialPrincipalToSubject() 	dom/base/nsGlobalWindow.cpp
9 	xul.dll 	nsWindowWatcher::OpenWindowInternal(nsIDOMWindow *,char const *,char const *,char const *,bool,bool,bool,nsIArray *,nsIDOMWindow * *) 	embedding/components/windowwatcher/src/nsWindowWatcher.cpp
10 	xul.dll 	nsWindowWatcher::OpenWindow2(nsIDOMWindow *,char const *,char const *,char const *,bool,bool,bool,nsISupports *,nsIDOMWindow * *) 	embedding/components/windowwatcher/src/nsWindowWatcher.cpp
11 	xul.dll 	nsGlobalWindow::OpenInternal(nsAString_internal const &,nsAString_internal const &,nsAString_internal const &,bool,bool,bool,bool,bool,nsIArray *,nsISupports *,nsIPrincipal *,JSContext *,nsIDOMWindow * *) 	dom/base/nsGlobalWindow.cpp
12 	xul.dll 	nsGlobalWindow::OpenInternal(nsAString_internal const &,nsAString_internal const &,nsAString_internal const &,bool,bool,bool,bool,bool,nsIArray *,nsISupports *,nsIPrincipal *,JSContext *,nsIDOMWindow * *) 	dom/base/nsGlobalWindow.cpp
13 	xul.dll 	nsGlobalWindow::OpenJS(nsAString_internal const &,nsAString_internal const &,nsAString_internal const &,nsIDOMWindow * *) 	dom/base/nsGlobalWindow.cpp
14 	xul.dll 	NS_InvokeByIndex 	xpcom/reflect/xptcall/src/md/win32/xptcinvoke.cpp
15 	xul.dll 	XPC_WN_CallMethod(JSContext *,unsigned int,JS::Value *) 	js/xpconnect/src/XPCWrappedNativeJSOps.cpp
16 	mozjs.dll 	js::Invoke(JSContext *,JS::CallArgs,js::MaybeConstruct) 	js/src/vm/Interpreter.cpp
17 	mozjs.dll 	js::Invoke(JSContext *,JS::Value const &,JS::Value const &,unsigned int,JS::Value const *,JS::MutableHandle<JS::Value>) 	js/src/vm/Interpreter.cpp
18 	mozjs.dll 	JS::Call(JSContext *,JS::Handle<JS::Value>,JS::Handle<JS::Value>,JS::HandleValueArray const &,JS::MutableHandle<JS::Value>) 	js/src/jsapi.cpp
19 	xul.dll 	xpc::SandboxCallableProxyHandler::call(JSContext *,JS::Handle<JSObject *>,JS::CallArgs const &) 	js/xpconnect/src/Sandbox.cpp
20 	mozjs.dll 	js::Invoke(JSContext *,JS::CallArgs,js::MaybeConstruct) 	js/src/vm/Interpreter.cpp
21 	mozjs.dll 	Interpret 	js/src/vm/Interpreter.cpp
22 	mozjs.dll 	js::RunScript(JSContext *,js::RunState &) 	js/src/vm/Interpreter.cpp
23 	mozjs.dll 	js::Invoke(JSContext *,JS::CallArgs,js::MaybeConstruct) 	js/src/vm/Interpreter.cpp
24 	mozjs.dll 	js_fun_apply(JSContext *,unsigned int,JS::Value *) 	js/src/jsfun.cpp
25 	mozjs.dll 	js::Invoke(JSContext *,JS::CallArgs,js::MaybeConstruct) 	js/src/vm/Interpreter.cpp
26 	mozjs.dll 	js::Invoke(JSContext *,JS::Value const &,JS::Value const &,unsigned int,JS::Value const *,JS::MutableHandle<JS::Value>) 	js/src/vm/Interpreter.cpp
27 	mozjs.dll 	js::jit::DoCallFallback 	js/src/jit/BaselineIC.cpp
28 		@0x2c2552f5 	
29 		@0x9ed7dc8 	
30 		@0x911bd4e 	
31 		@0x1b8b9fd8 	
32 		@0x2c2508f4 	
33 	mozjs.dll 	EnterBaseline 	js/src/jit/BaselineJIT.cpp
34 	mozjs.dll 	js::jit::EnterBaselineMethod(JSContext *,js::RunState &) 	js/src/jit/BaselineJIT.cpp
35 	mozjs.dll 	js::RunScript(JSContext *,js::RunState &) 	js/src/vm/Interpreter.cpp
36 	mozjs.dll 	js::Invoke(JSContext *,JS::CallArgs,js::MaybeConstruct) 	js/src/vm/Interpreter.cpp
37 	mozjs.dll 	js::Invoke(JSContext *,JS::Value const &,JS::Value const &,unsigned int,JS::Value const *,JS::MutableHandle<JS::Value>) 	js/src/vm/Interpreter.cpp
38 	mozjs.dll 	JS::Call(JSContext *,JS::Handle<JS::Value>,JS::Handle<JS::Value>,JS::HandleValueArray const &,JS::MutableHandle<JS::Value>) 	js/src/jsapi.cpp
39 	xul.dll 	mozilla::dom::EventListener::HandleEvent(JSContext *,JS::Handle<JS::Value>,mozilla::dom::Event &,mozilla::ErrorResult &) 	obj-firefox/dom/bindings/EventListenerBinding.cpp
40 	xul.dll 	mozilla::dom::EventListener::HandleEvent<mozilla::dom::EventTarget *>(mozilla::dom::EventTarget * const &,mozilla::dom::Event &,mozilla::ErrorResult &,mozilla::dom::CallbackObject::ExceptionHandling) 	obj-firefox/dist/include/mozilla/dom/EventListenerBinding.h
41 	xul.dll 	mozilla::EventListenerManager::HandleEventSubType(mozilla::EventListenerManager::Listener *,nsIDOMEvent *,mozilla::dom::EventTarget *) 	dom/events/EventListenerManager.cpp
42 	xul.dll 	mozilla::EventListenerManager::HandleEventInternal(nsPresContext *,mozilla::WidgetEvent *,nsIDOMEvent * *,mozilla::dom::EventTarget *,nsEventStatus *) 	dom/events/EventListenerManager.cpp
43 	xul.dll 	mozilla::EventTargetChainItem::HandleEventTargetChain(nsTArray<mozilla::EventTargetChainItem> &,mozilla::EventChainPostVisitor &,mozilla::EventDispatchingCallback *,mozilla::ELMCreationDetector &) 	dom/events/EventDispatcher.cpp
44 	xul.dll 	mozilla::EventDispatcher::Dispatch(nsISupports *,nsPresContext *,mozilla::WidgetEvent *,nsIDOMEvent *,nsEventStatus *,mozilla::EventDispatchingCallback *,nsCOMArray<mozilla::dom::EventTarget> *) 	dom/events/EventDispatcher.cpp
45 	xul.dll 	PresShell::HandleEventInternal(mozilla::WidgetEvent *,nsEventStatus *) 	layout/base/nsPresShell.cpp
46 	xul.dll 	PresShell::HandleEventWithTarget(mozilla::WidgetEvent *,nsIFrame *,nsIContent *,nsEventStatus *) 	layout/base/nsPresShell.cpp
47 	xul.dll 	mozilla::EventStateManager::CheckForAndDispatchClick(nsPresContext *,mozilla::WidgetMouseEvent *,nsEventStatus *) 	dom/events/EventStateManager.cpp
48 	xul.dll 	mozilla::EventStateManager::PostHandleEvent(nsPresContext *,mozilla::WidgetEvent *,nsIFrame *,nsEventStatus *) 	dom/events/EventStateManager.cpp
49 	xul.dll 	PresShell::HandleEventInternal(mozilla::WidgetEvent *,nsEventStatus *) 	layout/base/nsPresShell.cpp
50 	xul.dll 	PresShell::HandlePositionedEvent(nsIFrame *,mozilla::WidgetGUIEvent *,nsEventStatus *) 	layout/base/nsPresShell.cpp
51 	xul.dll 	PresShell::HandleEvent(nsIFrame *,mozilla::WidgetGUIEvent *,bool,nsEventStatus *) 	layout/base/nsPresShell.cpp
52 	xul.dll 	nsViewManager::DispatchEvent(mozilla::WidgetGUIEvent *,nsView *,nsEventStatus *) 	view/src/nsViewManager.cpp
53 	xul.dll 	nsView::HandleEvent(mozilla::WidgetGUIEvent *,bool) 	view/src/nsView.cpp
54 	xul.dll 	nsWindow::DispatchEvent(mozilla::WidgetGUIEvent *,nsEventStatus &) 	widget/windows/nsWindow.cpp
55 	xul.dll 	nsWindow::DispatchWindowEvent(mozilla::WidgetGUIEvent *) 	widget/windows/nsWindow.cpp
56 	xul.dll 	nsWindow::DispatchMouseEvent(unsigned int,unsigned int,long,bool,short,unsigned short) 	widget/windows/nsWindow.cpp
57 	xul.dll 	nsWindow::ProcessMessage(unsigned int,unsigned int &,long &,long *) 	widget/windows/nsWindow.cpp
58 	xul.dll 	nsWindow::WindowProcInternal(HWND__ *,unsigned int,unsigned int,long) 	widget/windows/nsWindow.cpp
59 	xul.dll 	CallWindowProcCrashProtected 	xpcom/base/nsCrashOnException.cpp
60 	xul.dll 	nsWindow::WindowProc(HWND__ *,unsigned int,unsigned int,long) 	widget/windows/nsWindow.cpp
61 	user32.dll 	user32.dll@0x7834 	
62 	user32.dll 	user32.dll@0x7a9a 	
63 	user32.dll 	user32.dll@0x988e 	
64 	user32.dll 	user32.dll@0x98f1 	
65 	xul.dll 	nsAppShell::ProcessNextNativeEvent(bool) 	widget/windows/nsAppShell.cpp
66 	xul.dll 	nsBaseAppShell::OnProcessNextEvent(nsIThreadInternal *,bool,unsigned int) 	widget/xpwidgets/nsBaseAppShell.cpp
67 	xul.dll 	nsThread::ProcessNextEvent(bool,bool *) 	xpcom/threads/nsThread.cpp
68 	xul.dll 	NS_ProcessNextEvent(nsIThread *,bool) 	xpcom/glue/nsThreadUtils.cpp
69 	xul.dll 	mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate *) 	ipc/glue/MessagePump.cpp
70 	xul.dll 	MessageLoop::RunHandler() 	ipc/chromium/src/base/message_loop.cc
71 	xul.dll 	MessageLoop::Run() 	ipc/chromium/src/base/message_loop.cc
72 	xul.dll 	nsBaseAppShell::Run() 	widget/xpwidgets/nsBaseAppShell.cpp
73 	xul.dll 	nsAppShell::Run() 	widget/windows/nsAppShell.cpp
74 	xul.dll 	nsAppStartup::Run() 	toolkit/components/startup/nsAppStartup.cpp
75 	xul.dll 	XREMain::XRE_mainRun() 	toolkit/xre/nsAppRunner.cpp
76 	xul.dll 	XREMain::XRE_main(int,char * * const,nsXREAppData const *) 	toolkit/xre/nsAppRunner.cpp
77 	xul.dll 	XRE_main 	toolkit/xre/nsAppRunner.cpp
78 	firefox.exe 	do_main 	browser/app/nsBrowserApp.cpp
79 	firefox.exe 	NS_internal_main(int,char * *) 	browser/app/nsBrowserApp.cpp
80 	firefox.exe 	wmain 	toolkit/xre/nsWindowsWMain.cpp
81 	firefox.exe 	__tmainCRTStartup 	f:/dd/vctools/crt_bld/self_x86/crt/src/crtexe.c
82 	kernel32.dll 	kernel32.dll@0x1919f 	
83 	ntdll.dll 	ntdll.dll@0x4a8cb 	
84 	ntdll.dll 	ntdll.dll@0x4a8a1 	
=============================================================

More reports:
https://crash-stats.mozilla.com/report/list?signature=CreateNativeGlobalForInner

This is pretty low volume right now but it first showed up on 2014-04-11 so I'm calling this a regression. One comment mentions "Tweet right add-on fails with latest Nightly build; crashes browser immediately." Looking at the version history for this add-on, it hasn't been updated since June 2013 and only has a 618 users, so this may be a red herring.

https://addons.mozilla.org/en-US/firefox/addon/tweetright/

Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=690c810c8e3e&tochange=d8c1b10c3a3d

Adding QAWANTED to see if we can reproduce this and get a narrower pushlog.

Comment 1

[u279076]

(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #0)
> Adding QAWANTED to see if we can reproduce this and get a narrower pushlog.

Paul, can you please work on this?

Comment 2

[Josh Matthews [:jdm]]

We're apparently hitting this assertion:

>2192   // DOMWindow with nsEP is not supported, we have to make sure
>2193   // no one creates one accidentally.
>2194   nsCOMPtr<nsIExpandedPrincipal> nsEP = do_QueryInterface(aPrincipal);
>2195   MOZ_RELEASE_ASSERT(!nsEP, "DOMWindow with nsEP is not supported");

Comment 3

[Henrik Gemal]

I crash with this signature every time I click on the "invite friends to passwordbox" link in the Passwordbox extension.
using nightly build

Comment 4

[Gabor Krizsanits (INACTIVE)]

I'm not sure why we want to create about:blank with anything else than nullprincipal... I guess the fix here would be to change http://mxr.mozilla.org/mozilla-central/source/dom/base/nsGlobalWindow.cpp#2028 to use null instead of the subject principal in case of expanded principals too. Does that sound the right thing to do?

Comment 5

[Paul Silaghi, QA [:pauly]]

STR:
1. Install https://addons.mozilla.org/en-US/firefox/addon/tweetright/
2. Right click on any text/Share on Twitter

AR: Crash
https://crash-stats.mozilla.com/report/index/bp-df48cea5-3dfa-48bb-839b-ea2712140415
https://crash-stats.mozilla.com/report/index/bp-56134225-ecd4-43ed-bfcd-f6adc2140415

Comment 6

[Paul Silaghi, QA [:pauly]]

Last good revision: a9dd8675e776
First bad revision: b816eb9618bb
Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=a9dd8675e776&tochange=b816eb9618bb

Comment 7

[u279076]

Good work, Paul. It looks like this is caused by bug 821809.

Comment 8

[u279076]

Sorry, :gijs, that was an erroneous needinfo.

Comment 9

[Boris Zbarsky [:bzbarsky]]

> I'm not sure why we want to create about:blank with anything else than nullprincipal

Because if a web page has <iframe src="about:blank"> it then expects to be able to script that subframe.  Similarly, if a web page does |var win = window.open()| it then expects to be able to script |win|.

> Does that sound the right thing to do?

Yes.

Comment 10

[Boris Zbarsky [:bzbarsky]]

We may want to add a new function on nsIPrincipal that returns true for system and expanded principals, since we now need this check in at least two places, right?

Comment 11

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] from comment #10)
> We may want to add a new function on nsIPrincipal that returns true for
> system and expanded principals, since we now need this check in at least two
> places, right?

Yeah. nsContentUtils::IsSystemOrExpandedPrincipal(aPrin)?

Comment 12

[Boris Zbarsky [:bzbarsky]]

Worksforme.

Comment 13

[Gabor Krizsanits (INACTIVE)]

(In reply to Bobby Holley (:bholley) from comment #11)
> (In reply to Boris Zbarsky [:bz] from comment #10)
> > We may want to add a new function on nsIPrincipal that returns true for
> > system and expanded principals, since we now need this check in at least two
> > places, right?
> 
> Yeah. nsContentUtils::IsSystemOrExpandedPrincipal(aPrin)?

I would keep them separated (system/expanded). In fact I would prefer some kind of GetType function on nsIPrincipal so we can avoid having to QI all the time. We do nsEP check at more and more places, and sooner or later we might do it at some hot code too (and then we could avoid the QI at least in the common case). But for now I would go for nsContentUtils::IsExpandedPrincipal as we already have IsSystemPrincipal there.

Comment 14

[Gabor Krizsanits (INACTIVE)]

Attachment: part1: IsSystemOrExpandedPrincipal

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8407549 [details] [diff] [review]
part1: IsSystemOrExpandedPrincipal

r=me

Comment 16

[Gabor Krizsanits (INACTIVE)]

Attachment: part2: minor cleanup in docshell

This part is just an optional clean up...

Comment 17

[Gabor Krizsanits (INACTIVE)]

Attachment: part3: nsEP check for window.open

Comment 18

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8407550 [details] [diff] [review]
part2: minor cleanup in docshell

r=me

Comment 19

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8407551 [details] [diff] [review]
part3: nsEP check for window.open

>+        ok(false);

Needs a description.

>+        ok(/Permission denied/.exec(e.message));

Likewise.

Might be good to set the finishTest member before doing the open() call, so we're not depending on that being async.

r=me with those fixed.

Comment 20

[Gabor Krizsanits (INACTIVE)]

https://tbpl.mozilla.org/?tree=Try&rev=d924881c9865

Comment 21

[Gabor Krizsanits (INACTIVE)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/13ddfc1637df
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/0f5ced1716d7
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/8cbd6019a24a

Comment 22

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 8407551 [details] [diff] [review]
part3: nsEP check for window.open

I would also like to uplift part1 and part2, those patches do not contain any functional change.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 821809
User impact if declined: Calling window.open from content-script of add-ons causes a crash.
Testing completed (on m-c, etc.): Just have pushed to inbound with a test
Risk to taking this patch (and alternatives if risky): I don't know about any, it's a simple patch.
String or IDL/UUID changes made by this patch: none

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/13ddfc1637df
https://hg.mozilla.org/mozilla-central/rev/0f5ced1716d7
https://hg.mozilla.org/mozilla-central/rev/8cbd6019a24a

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/778ac619e999
https://hg.mozilla.org/releases/mozilla-aurora/rev/0a1aff2920b1
https://hg.mozilla.org/releases/mozilla-aurora/rev/78ec8ab4d775

Comment 26

[Paul Silaghi, QA [:pauly]]

(In reply to Paul Silaghi, QA [:pauly] from comment #5)
> STR:
> 1. Install https://addons.mozilla.org/en-US/firefox/addon/tweetright/
> 2. Right click on any text/Share on Twitter
Verified fixed 31.0a1 (2014-04-22), 30.0a2 (2014-04-22) Win 7 x64

Comment 27

[Jesper Kristensen]

My add-on has stopped working in recent nightlies (between April 10 and 20).

My add-on does the following in a content script:

var win = open('');
win.document.body.innerHTML = '...';
and so on, to open a window with information about the current page.

This does not work anymore with this error message in the browser console:
Permission denied to access property 'document'

In the nightlies between when it worked and when it failed with this message, this code made Firefox crash with a reference to this bug.

Is my problem caused by this bug? What should I do to ensure my add-on continues to work?

(The reason why I open a blank page and then manipulate it via the DOM is that bug 792479 prevents me from opening a page that is part of my add-on)

Comment 28

[Gabor Krizsanits (INACTIVE)]

Bobby, what do you think? This is a pretty annoying side effect of the fix...But don't really know what else could we do.

Jesper, you could use evalInWindow or unsafeWindow.eval for "open('')" as a workaround in the meanwhile. It is available in content-scripts without Components.utils.

Comment 29

[Bobby Holley (:bholley)]

Hm. It seems like for document.open from chrome/nsEP, we shouldn't try to inherit the owner at all, and just keep using the principal of the page. Boris, does that make sense?

Comment 30

[Boris Zbarsky [:bzbarsky]]

For document.open(), I agree.

But the steps in comment 27 don't involve document.open() right?  I mean, we could try inheriting the principal of the window that had open() called there...

Comment 31

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] from comment #30)
> For document.open(), I agree.
> 
> But the steps in comment 27 don't involve document.open() right?

Oh, I totally misread that. My bad.

> I mean, we
> could try inheriting the principal of the window that had open() called
> there...

Right, that we be the analogue of my suggestion in this case.

Comment 32

[Gabor Krizsanits (INACTIVE)]

Let's take this to a new Bug 1006044. I will give it a try in the near future and we'll see how it works out.

Comment 33

[Jesper Kristensen]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #28)
> Jesper, you could use evalInWindow or unsafeWindow.eval for "open('')" as a
> workaround in the meanwhile. It is available in content-scripts without
> Components.utils.

Thanks, I ended up with this to make it work (in both Firefox and Chrome):

var win = (window.unsafeWindow || window).open('');
win.document.body.innerHTML = '...';

Comment 34

[Boris Zbarsky [:bzbarsky]]

Note that this will allow the web page to change your open() call do to whatever the heck it wants and return any window it wants....

Comment 35

[Jesper Kristensen]

(In reply to Jesper Kristensen from comment #33)
> (In reply to Gabor Krizsanits [:krizsa :gabor] from comment #28)
> > Jesper, you could use evalInWindow or unsafeWindow.eval for "open('')" as a
> > workaround in the meanwhile. It is available in content-scripts without
> > Components.utils.
> 
> Thanks, I ended up with this to make it work (in both Firefox and Chrome):
> 
> var win = (window.unsafeWindow || window).open('');
> win.document.body.innerHTML = '...';

I had to modify this. It turns out that unsafeWindow.open('') breaks event listeners in the new window. I fixed it using new XPCNativeWrapper(unsafeWindow.open('')) instead.