Closed Bug 1257096 Opened 8 years ago Closed 8 years ago

Shutdown-CC crash with Promise that references an HTMLPropertiesCollection with an expando

Categories

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

defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: jruderman, Assigned: smaug)

References

Details

(Keywords: crash, testcase, Whiteboard: btpp-active)

Crash Data

Attachments

(5 files, 2 obsolete files)

Attached file testcase
1. Load the testcase
2. Immediately quit browser

Crash [@ mozilla::dom::HTMLPropertiesCollection::EnsureFresh]
Attached file stack
oh oh, this is rather bad.
Sure, HTMLPropertiesCollection needs some null checks to fix this particular error, but the
bad thing is Promise doing anything interesting in unlink.

I totally reviewed http://hg.mozilla.org/mozilla-central/rev/0620f5288812
The  issue is that now the Promise may start to bring almost-dead C++ objects back to life.


But this bug should be used for fixing HTMLPropertiesCollection.
mRoot and mNames need to be null checked in various places.
Blocks: 903419
Depends on: 1257172
We could change the Promise code to only do the error report init with an object if that object is either a JS Error or an Exception/DOMException.  Those are the only interesting cases anyway, I think.  Then it won't poke random objects.
Assignee: nobody → bugs
Attached patch patch (obsolete) — Splinter Review
Had to change the .webidl so that null can be returned safely
Attachment #8731376 - Flags: review?(amarchesini)
I really do think comment 4 is a better way to fix this...
oh, I totally misunderstood your comment in the other bug.
Comment on attachment 8731376 [details] [diff] [review]
patch

but I agree
Attachment #8731376 - Flags: review?(amarchesini)
Attachment #8731376 - Attachment is obsolete: true
But actually, how does that work. DOMException for example might keep any random DOM objects alive via expandos
Hmm.  So the real invariant here is that it should be possible to do the errorreport init without calling into arbitrary script.  I thought that would work with DOMException, but you might be right in the sense that people can define own properties on it that shadow the canonical getters and then all bets are off.

We'd need to change the "duck-typing" bit in the JS error report init to call some runtime callback first and get the info that way without invoking actual JS on the object.  Or something.  Probably a good idea anyway.
Oh, IsDuckTypedErrorObject is such a hack :/
This is all horrible code. I decided to stay away from JS engine stuff as much as possible. It is still unclear to me why that code is so ugly.
Exception vs. DOMException is also weird. Why they both have some same member variables...

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c1dc4bb93dea
Attachment #8739246 - Flags: review?(bzbarsky)
Comment on attachment 8739246 [details] [diff] [review]
insane_exception_reporting.diff

>+    // Since GetName and GetMessageMoz are non-virtual and they deal with

Or you could make them virtual....

>-  if (mState != Rejected || mHadRejectCallback || mResult.isUndefined()) {

Why this change?  Seems to me like for primitives we might want to go through the existing report.init() codepath (withoug the new boolean true).  Or is it unsafe for some reason?

>+      if (!report.init(cx, val, true)) {

Please use an enum for the third arg, right?  And generally needs SpiderMonkey peer review.

>+#include "mozilla/dom/DOMException.h"

Could just forward-declare mozilla::dom::Exception and only include this in the .cpp

r=me
Attachment #8739246 - Flags: review?(bzbarsky) → review+
(In reply to Boris Zbarsky [:bz] from comment #13)
> Comment on attachment 8739246 [details] [diff] [review]
> insane_exception_reporting.diff
> 
> >+    // Since GetName and GetMessageMoz are non-virtual and they deal with
> 
> Or you could make them virtual....
well the issue is that the setup is so bizarre that I couldn't quite figure out 

> 
> >-  if (mState != Rejected || mHadRejectCallback || mResult.isUndefined()) {
> 
> Why this change?  Seems to me like for primitives we might want to go
> through the existing report.init() codepath (withoug the new boolean true). 
> Or is it unsafe for some reason?
Well, we return early from the init() if we can't detect mResult being some kind of error.
But ok, I guess I could change the js engine part so that we just disable the horrible ducktype stuff in this case.
Do we even try to ducktype primitives?  How would that work anyway?
Hmm, but that toString handling is still tricky. I guess we'll need to require either proper error object or non-object from which toString should be safe.

Have to say ErrorReport::init is among the worst methods I've seen for a long while ;)
Waldo, just check the jsfriendapi.h/jsexn.cpp part.
Attachment #8739628 - Flags: review?(jwalden+bmo)
Attachment #8739628 - Flags: review?(bzbarsky)
Whiteboard: btpp-active
Comment on attachment 8739628 [details] [diff] [review]
insane_exception_reporting_2.diff

>+    RefPtr<DOMException> domExp;

You don't need this.  DOMException inherits from Exception, so it's totally ok to do:

       NS_FAILED(UNWRAP_OBJECT(DOMException, &val.toObject(), exp))) {

where exp is a RefPtr<Exception>.

r=me on the dom and xpconnect changes.
Attachment #8739628 - Flags: review?(bzbarsky) → review+
Comment on attachment 8739628 [details] [diff] [review]
insane_exception_reporting_2.diff

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

Setting aside the utter bogosity of all this, this isn't horrible, just needs a few little changes.

::: dom/base/DOMException.h
@@ +125,5 @@
> +    } else if (!aName.IsEmpty()) {
> +      aRetVal.Assign(aName);
> +    } else if (!aMessage.IsEmpty()) {
> +      aRetVal.Assign(aMessage);
> +    }

Seems like you should assert aRetVal.IsEmpty() here.

::: js/src/jsexn.cpp
@@ +783,5 @@
>      }
>  
> +    if (!reportp && allowedType == ExceptionOrPrimitive && exn.isObject()) {
> +      return false;
> +    }

This is IMO misplaced.  We can MOZ_ASSERT(!reportp) at the start of this method.  Given that, we should add

        if (!reportp && sniffingBehavior == NoSideEffects) {
            JS_ReportErrorNumber(cx, GetErrorMessage, nullptr, JSMSG_ERR_DURING_THROW);
            return false;
        }

after the assignment to |reportp| in the block above, and add

MSG_DEF(JSMSG_ERR_DURING_THROW,        0, JSEXN_ERR,         "an internal error occurred while throwing an exception")

to js.msg in the // Internal errors block.  (Note that every caller of this method clears pending exceptions, so it's okay to report here.)  That all makes clearer that this only takes effect for the |exn.isObject()| case.

::: js/src/jsfriendapi.h
@@ +1374,5 @@
>      ~ErrorReport();
>  
> +    enum AllowedType {
> +      AnyKindOfType,
> +      ExceptionOrPrimitive

This isn't really the right name for this, nor the right initializers.  What this method *does*, is disable all sniffing of a thrown object to attempt to extract information from it.  It doesn't really have anything to do with "allowed" types (except in a vague, unclear sense), nor does "exception or primitive"-versus-"any" really capture the distinction being made.

enum SniffingBehavior {
    WithSideEffects,
    NoSideEffects
};

@@ +1378,5 @@
> +      ExceptionOrPrimitive
> +    };
> +
> +    bool init(JSContext* cx, JS::HandleValue exn,
> +              AllowedType allowedType = AnyKindOfType);

Make |SniffingBehavior sniffingBehavior| required, and modify the other callers.  I would rather every user deliberately decide what to pass for this, and every reader of that code recognize the choice made.

==

I don't fully understand what this API does with the new argument (not even reading the rest of the patch for background).  It's some sort of thing to initialize an error report from a value, but what exactly "initialize" means is extremely non-obvious.  The enum naming also leaves unclear what would happen if |exn| were a plain old object, when |ExceptionOrPrimitive| is passed.

Please add this API comment to clarify any of this:

"""
Generate a JSErrorReport from the provided thrown value.
 
If the value is a (possibly wrapped) Error object, the JSErrorReport will be exactly initialized from the Error object's information, without observable side effects.  (The Error object's JSErrorReport is reused, if it has one.)
 
Otherwise various attempts are made to derive JSErrorReport information from |exn| and from the current execution state.  This process is *definitely* inconsistent with any standard, and particulars of the behavior implemented here generally shouldn't be relied upon.
 
If the value of |sniffingBehavior| is |WithSideEffects|, some of these attempts *may* invoke user-configurable behavior when |exn| is an object: converting |exn| to a string, detecting and getting properties on |exn|, accessing |exn|'s prototype chain, and others are possible.  Users *must* tolerate |ErrorReport::init| potentially having arbitrary effects.  Any exceptions thrown by these operations will be caught and silently ignored, and "default" values will be substituted into the JSErrorReport.
 
But if the value of |sniffingBehavior| is |NoSideEffects|, these attempts *will not* invoke any observable side effects.  The JSErrorReport will simply contain fewer, less precise details.
 
Unlike some functions involved in error handling, this function adheres to the usual JSAPI return value error behavior.
"""

::: js/xpconnect/src/nsXPConnect.cpp
@@ +21,5 @@
>  #include "AccessCheck.h"
>  
>  #include "mozilla/dom/BindingUtils.h"
>  #include "mozilla/dom/Exceptions.h"
> +#include "mozilla/dom/DOMException.h"

Don't lose alphabetical order here?
Attachment #8739628 - Flags: review?(jwalden+bmo) → review-
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #21)
> @@ +125,5 @@
> > +    } else if (!aName.IsEmpty()) {
> > +      aRetVal.Assign(aName);
> > +    } else if (!aMessage.IsEmpty()) {
> > +      aRetVal.Assign(aMessage);
> > +    }
> 
> Seems like you should assert aRetVal.IsEmpty() here.
No. It is perfectly legal to have it empty, and we don't assert in other places either where we
check name and message. warning might make sense.
Based on IRC comment I may have misunderstood where you want the assertion and why. And still don't understand.
Attached patch v3 (obsolete) — Splinter Review
I haven't managed to get JS_ReportErrorNumber to actually do anything, I mean I don't see the error reported anywhere. Perhaps there is some magical JS engine pref to enable the reporting?
I was using first JSEXN_ERR, but changed to JSEXN_INTERNALERR to be consistent with other similar stuff, but that didn't seem to have any effect.
The error message for JSMSG_ERR_DURING_THROW is a bit wrong. It should probably say something about reporting, not throwing.
Attachment #8742969 - Flags: review?(jwalden+bmo)
Attached patch v4Splinter Review
Attachment #8742969 - Attachment is obsolete: true
Attachment #8742969 - Flags: review?(jwalden+bmo)
Attachment #8742979 - Flags: review?(jwalden+bmo)
Attachment #8742979 - Flags: review?(jwalden+bmo) → review+
https://hg.mozilla.org/mozilla-central/rev/49293302ac51
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
See Also: → 1486130
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.