Make exceptions thread agnostic

RESOLVED FIXED in Firefox 26

Status

()

defect
--
critical
RESOLVED FIXED
6 years ago
2 years ago

People

(Reporter: kairo, Assigned: khuey)

Tracking

({crash, dev-doc-needed, regression})

Trunk
mozilla26
x86
Windows NT
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox25 unaffected, firefox26 fixed)

Details

(crash signature)

Attachments

(8 attachments, 2 obsolete attachments)

6.21 KB, patch
bholley
: review+
Details | Diff | Splinter Review
15.13 KB, patch
benjamin
: review+
Details | Diff | Splinter Review
2.39 KB, patch
khuey
: review+
Details | Diff | Splinter Review
30.87 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
55.00 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
13.54 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
97.05 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
89.40 KB, patch
Details | Diff | Splinter Review
This bug was filed from the Socorro interface and is 
report bp-1b806960-41fb-4715-bb22-d162d2130830.
=============================================================

Top frames (see report for more):
0 	xul.dll 	nsXPCException::NewException(char const *,tag_nsresult,nsIStackFrame *,nsISupports *,nsIException * *) 	js/xpconnect/src/XPCException.cpp
1 	xul.dll 	XPCThrower::BuildAndThrowException(JSContext *,tag_nsresult,char const *) 	js/xpconnect/src/XPCThrower.cpp
2 	xul.dll 	XPCThrower::Throw(tag_nsresult,JSContext *) 	js/xpconnect/src/XPCThrower.cpp
3 	xul.dll 	xpc::Throw(JSContext *,tag_nsresult) 	js/xpconnect/src/XPCThrower.cpp
4 	xul.dll 	mozilla::dom::Throw<1>(JSContext *,tag_nsresult) 	obj-firefox/dist/include/mozilla/dom/BindingUtils.h
5 	xul.dll 	mozilla::dom::ThrowMethodFailedWithDetails<1>(JSContext *,mozilla::ErrorResult &,char const *,char const *) 	obj-firefox/dist/include/mozilla/dom/BindingUtils.h
6 	xul.dll 	mozilla::dom::TextEncoderBinding::encode 	obj-firefox/dom/bindings/TextEncoderBinding.cpp
7 	xul.dll 	mozilla::dom::TextEncoderBinding::genericMethod 	obj-firefox/dom/bindings/TextEncoderBinding.cpp
8 	mozjs.dll 	js::Invoke(JSContext *,JS::CallArgs,js::MaybeConstruct) 	js/src/vm/Interpreter.cpp
9 	mozjs.dll 	Interpret 	js/src/vm/Interpreter.cpp
10 	mozjs.dll 	js::RunScript(JSContext *,js::RunState &) 	js/src/vm/Interpreter.cpp


Not sure if that belongs in XPConnet or actually in whatever component that mozilla::dom::TextEncoderBinding::encode belongs in, or something else entirely.

This is a new crash that started happening on 2013-08-25 and since has instances on 26.0a1 Nightly every day. It ranges at #18 over the last 7 days of builds, so far happens on Windows only, seems like something to look into as it has a clear regression day, though.

https://crash-stats.mozilla.com/report/list?signature=nsXPCException%3A%3ANewException%28char%20const%2A%2C%20tag_nsresult%2C%20nsIStackFrame%2A%2C%20nsISupports%2A%2C%20nsIException%2A%2A%29
I looked at three of these stacks, and 2 out of 3 were happening on worker threads, so this could be a regression from the worker text encoder stuff.  I don't have the bug number for that handy so I'm not sure what it landed.
Ah, yes, TextEncoder can actually throw.

Crap.
Assignee: nobody → khuey
Blocks: 853893, 903770
Summary: crash in nsXPCException::NewException → Make exceptions thread agnostic
It's not doing anything useful, so let's get rid of it.
Attachment #798935 - Flags: review?(bobbyholley+bmo)
Attachment #798937 - Attachment description: Kill the exception service → Part 2: Kill the exception service
Not really sure why I made this separate ... it should be folded into the following patch.
Attachment #798939 - Flags: review+
This converts both regular XPConnect exceptions and DOM exceptions to webidl, and makes the DOM exception impl inherit from the XPConnect impl.
Attachment #798941 - Flags: review?(bzbarsky)
Posted patch Part 5 - Refactor exceptions (obsolete) — Splinter Review
This moves the exception implementations to mozilla::dom and pushes the pending exception handling down into CycleCollectedJSRuntime.

Longer term I think we want to figure out what to do with code that is shared between XPConnect and workers.  dom/ seems like a fine home to me but bholley may disagree.
Attachment #798943 - Flags: review?(bzbarsky)
Finish making exception handling thread-agnostic and use it everywhere.
Attachment #798945 - Flags: review?(bzbarsky)
Comment on attachment 798935 [details] [diff] [review]
Part 1 - Stop using nsIExceptionManager

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

::: js/xpconnect/src/XPCThrower.cpp
@@ +182,5 @@
> +    XPCJSRuntime* runtime = XPCJSRuntime::Get();
> +    nsCOMPtr<nsIException> existingException = runtime->GetPendingException();
> +    if (existingException) {
> +        nsresult nr;
> +        if (NS_SUCCEEDED(existingException->GetResult(&nr)) && 

whitespace error

@@ +196,5 @@
> +                                 getter_AddRefs(defaultException));
> +
> +    // Do we use DOM exceptions for this error code?
> +    switch (NS_ERROR_GET_MODULE(rv)) {
> +    case NS_ERROR_MODULE_DOM:

Indentation.
Attachment #798935 - Flags: review?(bobbyholley+bmo) → review+
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #7)
> Longer term I think we want to figure out what to do with code that is
> shared between XPConnect and workers.  dom/ seems like a fine home to me but
> bholley may disagree.

xpcom/ for non-dom machinery, dom/ for dom machinery IMO.
Blocks: 910910
Comment on attachment 798941 [details] [diff] [review]
Part 4 - Convert exceptions to WebIDL

So the constructor expects static strings, right?  Please document that.

>+nsDOMException::GetName(nsString& retval)
>+  retval = NS_ConvertUTF8toUTF16(mName);

  NS_CopyUTF8toUTF16(mName, retval);

and similar lots of other places in this patch.

Is there a reason nsXPCException doesn't use NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE_0?

We may want to keep the GetMessageMoz binaryname, to avoid Windows troubles.

>+  readonly attribute nsISupports             location;
>+  readonly attribute nsISupports             inner;
>+  readonly attribute nsISupports             data;

All three should be nullable, no?

>@@ -190,10 +191,7 @@ XPCThrower::BuildAndThrowException(JSContext* cx,
>-            finalException = NS_NewDOMException(rv, defaultException);

Didn't that use to give the DOMException the right filename/lineno, etc?  How is that working now?

This patch seems to be assuming that Initialize() never returns error, nor do the various nsXPCException XPCOM methods.  I assume you've audited that?  Could we at least add asserts or something?

r=me with that.  Followup to remove the classinfo?
Attachment #798941 - Flags: review?(bzbarsky) → review+
The following fixes are required to pass tests:

1. Don't use nsContentUtils::GetCurrentJSContext, since nsContentUtils is not initialized in xpcshell but XPConnect still needs to throw exceptions there.
2. Avoid clobbering an existing exception in Throw to allow ancient components that throw custom exception strings (such as plugins and crypto) to continue to do so.
3. Use new binding unwrapping to make instanceof Components.Exception continue to work.
4. Use new binding unwrapping when looking to see if an XPCWrappedJS threw NS_NOINTERFACE so that we continue to properly swallow NS_NOINTERFACE exceptions.
5. Change a few tests to use SpecialPowers.wrap on exception objects now that exceptions have a canonical wrapper.
6. Explicitly clear mPendingException in ~CycleCollectedJSRuntime so that if a pending exception is cycle collected we clear it before we shut down the cycle collector (since exceptions are now cycle collected, not doing this crashes).
Attachment #799560 - Flags: review?(bzbarsky)
Comment on attachment 798943 [details] [diff] [review]
Part 5 - Refactor exceptions

Please post a patch with "hg mv" instead of delete and readd.  That would also incidentally be simpler to review....
Attachment #798943 - Flags: review?(bzbarsky) → review-
Comment on attachment 798945 [details] [diff] [review]
Part 6 - Use thread-agnostic exception handling everywhere

This needs a way better checkin comment.

Assuming exceptions::CreateStack is threadsafe, r=me
Attachment #798945 - Flags: review?(bzbarsky) → review+
(In reply to Boris Zbarsky [:bz] from comment #13)
> Comment on attachment 798943 [details] [diff] [review]
> Part 5 - Refactor exceptions
> 
> Please post a patch with "hg mv" instead of delete and readd.  That would
> also incidentally be simpler to review....

mmm, git doesn't really do that :-/  I'll see what I can do
Comment on attachment 799560 [details] [diff] [review]
Part 7 - Fixes to pass tests

>+    // We can't call nsContentUtils::ThreadsafeGetCurrentJSContext

Because it doesn't exist after this patch.  Did you mean nsContentUtils::GetCurrentJSContext?

>+        *bp = UNWRAP_OBJECT(Exception, cx, obj, e) ||
>+              JSValIsInterfaceOfType(cx, v, NS_GET_IID(nsIException));

UNWRAP_OBJECT returns nsresult.  So you want NS_SUCCEEDED() around it.

>+    e = SpecialPowers.wrap(e);

Maybe file a followup to have the specialpowers wrappers auto-do that when exceptions happen?

Can you fold the pieces of this into the relevant patches, please, insofar as that's possible?

r=me with that.
Attachment #799560 - Flags: review?(bzbarsky) → review+
(In reply to Boris Zbarsky [:bz] from comment #16)
> Comment on attachment 799560 [details] [diff] [review]
> Part 7 - Fixes to pass tests
> 
> >+    // We can't call nsContentUtils::ThreadsafeGetCurrentJSContext
> 
> Because it doesn't exist after this patch.  Did you mean
> nsContentUtils::GetCurrentJSContext?

Yes.

> >+        *bp = UNWRAP_OBJECT(Exception, cx, obj, e) ||
> >+              JSValIsInterfaceOfType(cx, v, NS_GET_IID(nsIException));
> 
> UNWRAP_OBJECT returns nsresult.  So you want NS_SUCCEEDED() around it.

Yeah, try pointed that out to me too.

> >+    e = SpecialPowers.wrap(e);
> 
> Maybe file a followup to have the specialpowers wrappers auto-do that when
> exceptions happen?

Ok.

> Can you fold the pieces of this into the relevant patches, please, insofar
> as that's possible?

Yes, I will.
Attachment #798937 - Flags: review?(benjamin) → review+
With moves, but since I reformatted everything I moved out of XPConnect to gecko style I'm not sure it helps that much :-/
Attachment #798943 - Attachment is obsolete: true
Attachment #800244 - Flags: review?(bzbarsky)
(In reply to Boris Zbarsky [:bz] from comment #11)
> Comment on attachment 798941 [details] [diff] [review]
> Part 4 - Convert exceptions to WebIDL
> 
> So the constructor expects static strings, right?  Please document that.

Done.

> >+nsDOMException::GetName(nsString& retval)
> >+  retval = NS_ConvertUTF8toUTF16(mName);
> 
>   NS_CopyUTF8toUTF16(mName, retval);
> 
> and similar lots of other places in this patch.

Fixed.  (didn't know that signature existed).

> Is there a reason nsXPCException doesn't use
> NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE_0?

It gets a jsval it has to trace later in the series.

> We may want to keep the GetMessageMoz binaryname, to avoid Windows troubles.

Changed.

> >+  readonly attribute nsISupports             location;
> >+  readonly attribute nsISupports             inner;
> >+  readonly attribute nsISupports             data;
> 
> All three should be nullable, no?

Indeed.  Fixed.

> >@@ -190,10 +191,7 @@ XPCThrower::BuildAndThrowException(JSContext* cx,
> >-            finalException = NS_NewDOMException(rv, defaultException);
> 
> Didn't that use to give the DOMException the right filename/lineno, etc? 
> How is that working now?

Inheritance.  The Exception ctor grabs the necessary stuff.

> This patch seems to be assuming that Initialize() never returns error, nor
> do the various nsXPCException XPCOM methods.  I assume you've audited that? 
> Could we at least add asserts or something?

Added.

> r=me with that.  Followup to remove the classinfo?

Will do.
(In reply to Boris Zbarsky [:bz] from comment #14)
> Comment on attachment 798945 [details] [diff] [review]
> Part 6 - Use thread-agnostic exception handling everywhere
> 
> This needs a way better checkin comment.

So do all the other patches ;-)

> Assuming exceptions::CreateStack is threadsafe, r=me
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #19)
> > >+nsDOMException::GetName(nsString& retval)
> > >+  retval = NS_ConvertUTF8toUTF16(mName);
> > 
> >   NS_CopyUTF8toUTF16(mName, retval);
> > 
> > and similar lots of other places in this patch.
> 
> Fixed.  (didn't know that signature existed).

Ah, it doesn't.  But there is a CopyUTF8toUTF16 that does what I want.
> but since I reformatted everything I moved out of XPConnect to gecko style

diff -w?  ;)
Posted patch Part 5: diff -w (obsolete) — Splinter Review
Doesn't shrink it much :-/
Posted patch Part 5Splinter Review
This won't compile but I think it's easier to review.
Attachment #800554 - Attachment is obsolete: true
This will affect documentation.
Keywords: dev-doc-needed
Comment on attachment 800244 [details] [diff] [review]
Part 5 - Refactor exceptions

>+++ b/dom/base/DOMException.h
>+#pragma GCC diagnostic push
>+#pragma GCC diagnostic ignored "-Woverloaded-virtual"

Please add a comment explaining why.

>+#define MOZILLA_DOM_EXCEPTION_IID \

Should that perhaps be called MOZILLA_EXCEPTION_IID?

>+++ b/dom/bindings/Exceptions.cpp
>+ThrowExceptionObject(JSContext* aCx, nsIException* aException)

>+  JS::RootedObject glob(aCx, JS::CurrentGlobalOrNull(aCx));

JS::Rooted<JSObject*>, please; I think that's current bindings style.

>+  nsresult rv = nsContentUtils::XPConnect()->WrapNative(aCx, glob, aException,

How about:
  
  JS::Rooted<JS::Value> val(aCx);
  if (!WrapObject(aCx, glob, aException, NS_GET_IID(nsIException), &val)) {
    return false;
  }

and skipping the explicit xpconnect bits?

>+ThrowExceptionObject(JSContext* aCx, Exception* aException)
>+  JS::RootedValue thrown(aCx);

JS::Rooted<JS::Value>.

>+  JS::RootedObject glob(aCx, JS::CurrentGlobalOrNull(aCx));

JS::Rooted<JSObject*>.

>+  JS::RootedValue val(aCx);

Why not keep using |thrown|?

>+  if (!mozilla::dom::WrapNewBindingObject(aCx, glob, aException, &val)) {

You're already in namespace mozilla::dom.

>+Throw(JSContext* cx, nsresult rv, const char* sz)

Give that last argument a sane name, please.  

>+  if (finalException == nullptr) {

if (!finalException)

>+NS_IMETHODIMP JSStackFrame::ToString(char** _retval)
>+            sizeof(format) + 6 /* space for lineno */;

I know you just copied this, but mind doing +10 or something, since lineno is 32-bit?  Or better yet +3*sizeof(mLineNo) to be very safe even if mLineNo goes 64-bit?

>+    self = frame.forget();

self.swap(frame) might be clearer.

>+++ b/js/xpconnect/src/XPCConvert.cpp
>+    if (cx && jsExceptionPtr) {
>+    e.forget(exceptn);

These two changes should have been in part 4, right?

>+++ b/dom/base/DOMException.cpp
>+Exception::~Exception()
>+    NS_DROP_JS_OBJECTS(this, Exception);

Will need merging.

>+Exception::StealJSVal(JS::Value* aVp)
>+    mThrownJSVal = JSVAL_NULL;

  mThrownJSVal.setNull();

Followup to make this take a MutableHandle, perhaps?

>+    NS_DROP_JS_OBJECTS(this, Exception);

Will need merging.

>+Exception::StowJSVal(JS::Value& aVp)

And maybe followup to take a Handle here?

>+    NS_HOLD_JS_OBJECTS(this, Exception);

Will need merging.

>+++ b/js/xpconnect/src/XPCQuickStubs.cpp
>-    XPCThrower::BuildAndThrowException(cx, rv, sz);
>+    dom::Throw(cx, rv, sz);

The old thing wouldn't clobber an existing exception but it looks like the new one does.  Why the change?  There are several places where this comment applies, both in this file and in XPCThrower.

>+++ b/xpcom/base/CycleCollectedJSRuntime.h
>+  already_AddRefed<nsIException> GetPendingException() const;

Why not return nsIException*?  Worth documenting if there is a reason.

>+  static CycleCollectedJSRuntime*
>+  Get();

Put that on one line?

r=me with those nits picked
Attachment #800244 - Flags: review?(bzbarsky) → review+
Please double-check the bug numbers in your commit messages.
(In reply to Boris Zbarsky [:bz] from comment #26)
> Comment on attachment 800244 [details] [diff] [review]
> Part 5 - Refactor exceptions
> 
> >+++ b/dom/base/DOMException.h
> >+#pragma GCC diagnostic push
> >+#pragma GCC diagnostic ignored "-Woverloaded-virtual"
> 
> Please add a comment explaining why.

Done.

> >+#define MOZILLA_DOM_EXCEPTION_IID \
> 
> Should that perhaps be called MOZILLA_EXCEPTION_IID?

Yeah, I suppose so, since there's a DOMException too.

> >+++ b/dom/bindings/Exceptions.cpp
> >+ThrowExceptionObject(JSContext* aCx, nsIException* aException)
> 
> >+  JS::RootedObject glob(aCx, JS::CurrentGlobalOrNull(aCx));
> 
> JS::Rooted<JSObject*>, please; I think that's current bindings style.

Changed.

> >+  nsresult rv = nsContentUtils::XPConnect()->WrapNative(aCx, glob, aException,
> 
> How about:
>   
>   JS::Rooted<JS::Value> val(aCx);
>   if (!WrapObject(aCx, glob, aException, NS_GET_IID(nsIException), &val)) {
>     return false;
>   }
> 
> and skipping the explicit xpconnect bits?

AFAICT WrapObject only accepts things inheriting from nsWrapperCache, and nsIException doesn't (and we might have an XPCWrappedJS here, so there's no guarantee that we really do inherit from nsWrapperCache).  If I missed something please file a followup and I'll fix it.

> >+ThrowExceptionObject(JSContext* aCx, Exception* aException)
> >+  JS::RootedValue thrown(aCx);
> 
> JS::Rooted<JS::Value>.

Changed.

> >+  JS::RootedObject glob(aCx, JS::CurrentGlobalOrNull(aCx));
> 
> JS::Rooted<JSObject*>.

Same.

> >+  JS::RootedValue val(aCx);
> 
> Why not keep using |thrown|?

No reason.  Fixed.

> >+  if (!mozilla::dom::WrapNewBindingObject(aCx, glob, aException, &val)) {
> 
> You're already in namespace mozilla::dom.

Indeed.

> >+Throw(JSContext* cx, nsresult rv, const char* sz)
> 
> Give that last argument a sane name, please. 

Fixed. 
> 
> >+  if (finalException == nullptr) {
> 
> if (!finalException)
> 
> >+NS_IMETHODIMP JSStackFrame::ToString(char** _retval)
> >+            sizeof(format) + 6 /* space for lineno */;
> 
> I know you just copied this, but mind doing +10 or something, since lineno
> is 32-bit?  Or better yet +3*sizeof(mLineNo) to be very safe even if mLineNo
> goes 64-bit?

Done.

> >+    self = frame.forget();
> 
> self.swap(frame) might be clearer.

Fixed.

> >+++ b/js/xpconnect/src/XPCConvert.cpp
> >+    if (cx && jsExceptionPtr) {
> >+    e.forget(exceptn);
> 
> These two changes should have been in part 4, right?

Not really.  I didn't remove these methods from the interface until this patch.  Although these could have been in part 4 if I had changed the concrete type of the pointer.

> >+++ b/dom/base/DOMException.cpp
> >+Exception::~Exception()
> >+    NS_DROP_JS_OBJECTS(this, Exception);
> 
> Will need merging.
> 
> >+Exception::StealJSVal(JS::Value* aVp)
> >+    mThrownJSVal = JSVAL_NULL;
> 
>   mThrownJSVal.setNull();
> 
> Followup to make this take a MutableHandle, perhaps?

Ok.

> >+    NS_DROP_JS_OBJECTS(this, Exception);
> 
> Will need merging.
> 
> >+Exception::StowJSVal(JS::Value& aVp)
> 
> And maybe followup to take a Handle here?
> 
> >+    NS_HOLD_JS_OBJECTS(this, Exception);
> 
> Will need merging.
> 
> >+++ b/js/xpconnect/src/XPCQuickStubs.cpp
> >-    XPCThrower::BuildAndThrowException(cx, rv, sz);
> >+    dom::Throw(cx, rv, sz);
> 
> The old thing wouldn't clobber an existing exception but it looks like the
> new one does.  Why the change?  There are several places where this comment
> applies, both in this file and in XPCThrower.

This is fixed in part 7 and folded back in appropriately.

> >+++ b/xpcom/base/CycleCollectedJSRuntime.h
> >+  already_AddRefed<nsIException> GetPendingException() const;
> 
> Why not return nsIException*?  Worth documenting if there is a reason.

All the callers wanted a strong reference.  *shrug*

> >+  static CycleCollectedJSRuntime*
> >+  Get();
> 
> Put that on one line?

Fixed.

> r=me with those nits picked

Thanks.
> AFAICT WrapObject only accepts things inheriting from nsWrapperCache

That's incorrect.  I'll call into the normal xpconnect wrapping code via XPCOMObjectToJsval.  Make sure to call this signature:

1037 template<class T>
1038 inline bool
1039 WrapObject(JSContext* cx, JS::Handle<JSObject*> scope, T* p, const nsIID* iid,
1040            JS::MutableHandle<JS::Value> rval)

and note no wrapper cache requirement there.  I filed bug 914014 on this.

> Not really.  I didn't remove these methods from the interface until this patch. 

I'm not sure what you mean.  If I'm reading the code right, part 4 as-attached (part 3 as-landed) never sets *exceptn.  It then tests *exceptn which is either null (probably) or uninitialized (less likely).  If, during a bisect, you happen to build that changeset it won't behave right if this code is ever hit.  Not much we can do about it now that it's landed.  :(
Depends on: 914014
KaiRo can you watch crashstats here and make sure this goes away?
Flags: needinfo?(kairo)
Blocks: 915202
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #33)
> KaiRo can you watch crashstats here and make sure this goes away?

The "Table" tab in https://crash-stats.mozilla.com/report/list?signature=nsXPCException%3A%3ANewException(char+const*%2C+tag_nsresult%2C+nsIStackFrame*%2C+nsISupports*%2C+nsIException**) says the last crashes with that were in the build from September 9, so the fix seems effective in stopping this signature.
Flags: needinfo?(kairo)
Depends on: 928655
Depends on: 1218437
You need to log in before you can comment on or make changes to this bug.