Closed
Bug 933574
Opened 11 years ago
Closed 11 years ago
ReportPendingException is using wrong compartment
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
RESOLVED
DUPLICATE
of bug 872273
People
(Reporter: gwagner, Unassigned)
References
Details
Attachments
(1 file, 3 obsolete files)
(deleted),
patch
|
bholley
:
review-
|
Details | Diff | Splinter Review |
Errors from indexeddb are getting converted into:
E/GeckoConsole( 1626): [JavaScript Error: "Permission denied to access property 'toString'"]
E/GeckoConsole( 1626): [JavaScript Error: "Permission denied to access property 'message'"]
E/GeckoConsole( 1626): [JavaScript Error: "uncaught exception: unknown (can't convert to string)"]
Reporter | ||
Comment 1•11 years ago
|
||
mrbkap and bent fixed this one.
Reporter | ||
Updated•11 years ago
|
Attachment #825682 -
Flags: review?(bzbarsky)
Comment 2•11 years ago
|
||
Comment on attachment 825682 [details] [diff] [review]
patch
Review of attachment 825682 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/base/nsJSUtils.cpp
@@ +140,5 @@
> {
> if (JS_IsExceptionPending(aContext)) {
> bool saved = JS_SaveFrameChain(aContext);
> {
> + JS::RootedValue exn(aContext);
JS::Rooted<JS::Value>.
@@ +143,5 @@
> {
> + JS::RootedValue exn(aContext);
> + JS_GetPendingException(aContext, &exn);
> +
> + JS::RootedObject scope(aContext);
JS::Rooted<JSObject*>.
Reporter | ||
Comment 3•11 years ago
|
||
I think Bobby wrote this code.
Attachment #825682 -
Attachment is obsolete: true
Attachment #825682 -
Flags: review?(bzbarsky)
Attachment #826047 -
Flags: review?(bobbyholley+bmo)
Reporter | ||
Comment 4•11 years ago
|
||
Attachment #826047 -
Attachment is obsolete: true
Attachment #826047 -
Flags: review?(bobbyholley+bmo)
Attachment #826049 -
Flags: review?(bobbyholley+bmo)
Reporter | ||
Comment 5•11 years ago
|
||
Its friday...
Attachment #826049 -
Attachment is obsolete: true
Attachment #826049 -
Flags: review?(bobbyholley+bmo)
Attachment #826068 -
Flags: review?(bobbyholley+bmo)
Comment 6•11 years ago
|
||
Comment on attachment 826068 [details] [diff] [review]
patch
Review of attachment 826068 [details] [diff] [review]:
-----------------------------------------------------------------
/me has been eying this patch suspiciously for a few days now, and has decided that it's not kosher.
If someone throws |new Error()|, we compute a JSErrorReport on the spot, and stash it in a private slot on the object that bubbles up the stack. If it gets all the way up, we invoke JS_ReportPendingException(cx), which delegates to js_ReportUncaughtException(cx).
If the exception is a bonafide Error object (security-wrapped or not), we pull out the JSErrorReport, and pass it along to DOM script error reporter. Unwrapping the security wrapper is fine, because the JSErrorReport has an originPrincipals associated with it, which means it gets properly sanitized when passing to any any handler that is not same-origin with the origin of the _script_ (not the page) that threw the error.
If the exception is not bonafide Error object, we try to duck-type it, and generate an error report manually (with a null originPrincipals). This duck-typing is ok, because for cross-origin exception objects, we'll have a security wrapper that filters things appropriately.
But this all depends on the security wrappers encountered during duck-typing being the ones that would be seen by the scope in which the onerror handler might be running, which is that of the current inner window of the nsJSContext of whatever cx we're reporting the exception on. If we enter the compartment of the error object, we can leak information that the onerror handler isn't supposed to see. For example, if you did |throw crossOriginWindow.location|, and it passed duck-typing somehow, you'd report the stringification of the cross-origin location object to the onerror handler, which is a chemspill-worth offense.
Interestingly, this patch as it stands isn't actually always going to enter its intended compartment, because the pending exception is rewrapped on each compartment enter/leave. The only reason it's at all different in this case is that the JS_SaveFrameChain does cx->setCompartment(null), which means that the exception gets left in whatever scope we happened to be in before the call to nsJSUtils::ReportPendingException, which varies depending on the caller.
Boris, Waldo - does that all seem right?
Attachment #826068 -
Flags: review?(bobbyholley+bmo) → review-
Comment 7•11 years ago
|
||
And in terms of the bug you're trying to solve, I think you basically want bug 872273, which I never managed to land.
Reporter | ||
Comment 8•11 years ago
|
||
So what are the next steps here? Bobby, are you going to finish bug 872273?
We hit assertions during tests and we have no good way to get the actual content.
Like here for example: https://tbpl.mozilla.org/php/getParsedLog.php?id=30444652&tree=Cedar
Flags: needinfo?(bobbyholley+bmo)
Comment 9•11 years ago
|
||
I will fix bug 872273 if Waldo fixes bug 724768.
Flags: needinfo?(bobbyholley+bmo) → needinfo?(jwalden+bmo)
Comment 10•11 years ago
|
||
Fixing that now that there's definite need sounds good to me. It should be fairly quick, I'd guess on the order of a day or two. I have some backporting of other patches to do, but after that I can pick that up again. Guessing patches will be ready there by end of next week, if not sooner.
Flags: needinfo?(jwalden+bmo)
Comment 12•11 years ago
|
||
(In reply to Gregor Wagner [:gwagner] from comment #11)
> Is this now a dup of 872273?
I would think so, yes. If you run into any other problems in this area, please file. But I think this bug can be closed.
Status: NEW → RESOLVED
Closed: 11 years ago
Flags: needinfo?(bobbyholley)
Resolution: --- → DUPLICATE
Assignee | ||
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•