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)
Core
DOM: Core & HTML
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)
264 bytes,
text/html
|
Details | |
6.80 KB,
text/plain
|
Details | |
10.22 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
10.41 KB,
patch
|
bzbarsky
:
review+
Waldo
:
review-
|
Details | Diff | Splinter Review |
15.58 KB,
patch
|
Waldo
:
review+
|
Details | Diff | Splinter Review |
1. Load the testcase 2. Immediately quit browser Crash [@ mozilla::dom::HTMLPropertiesCollection::EnsureFresh]
Reporter | ||
Comment 1•8 years ago
|
||
Assignee | ||
Comment 2•8 years ago
|
||
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.
Assignee | ||
Comment 3•8 years ago
|
||
https://bugzilla.mozilla.org/show_bug.cgi?id=903419&mark=5-6#c5
Comment 4•8 years ago
|
||
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 | ||
Updated•8 years ago
|
Assignee: nobody → bugs
Assignee | ||
Comment 5•8 years ago
|
||
Had to change the .webidl so that null can be returned safely
Attachment #8731376 -
Flags: review?(amarchesini)
Assignee | ||
Comment 7•8 years ago
|
||
oh, I totally misunderstood your comment in the other bug.
Assignee | ||
Comment 8•8 years ago
|
||
Comment on attachment 8731376 [details] [diff] [review] patch but I agree
Attachment #8731376 -
Flags: review?(amarchesini)
Assignee | ||
Updated•8 years ago
|
Attachment #8731376 -
Attachment is obsolete: true
Assignee | ||
Comment 9•8 years ago
|
||
But actually, how does that work. DOMException for example might keep any random DOM objects alive via expandos
Comment 10•8 years ago
|
||
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.
Assignee | ||
Comment 11•8 years ago
|
||
Oh, IsDuckTypedErrorObject is such a hack :/
Assignee | ||
Comment 12•8 years ago
|
||
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 13•8 years ago
|
||
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+
Assignee | ||
Comment 14•8 years ago
|
||
(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.
Comment 15•8 years ago
|
||
Do we even try to ducktype primitives? How would that work anyway?
Assignee | ||
Comment 16•8 years ago
|
||
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 ;)
Assignee | ||
Comment 17•8 years ago
|
||
Waldo, just check the jsfriendapi.h/jsexn.cpp part.
Attachment #8739628 -
Flags: review?(jwalden+bmo)
Attachment #8739628 -
Flags: review?(bzbarsky)
Updated•8 years ago
|
Whiteboard: btpp-active
Comment 18•8 years ago
|
||
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 21•8 years ago
|
||
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-
Assignee | ||
Comment 22•8 years ago
|
||
(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.
Assignee | ||
Comment 23•8 years ago
|
||
Based on IRC comment I may have misunderstood where you want the assertion and why. And still don't understand.
Assignee | ||
Comment 24•8 years ago
|
||
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)
Assignee | ||
Comment 25•8 years ago
|
||
Attachment #8742969 -
Attachment is obsolete: true
Attachment #8742969 -
Flags: review?(jwalden+bmo)
Attachment #8742979 -
Flags: review?(jwalden+bmo)
Updated•8 years ago
|
Attachment #8742979 -
Flags: review?(jwalden+bmo) → review+
Assignee | ||
Comment 26•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/49293302ac51
Comment 27•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/49293302ac51
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•