As a security precaution, we have turned on the setting "Require API key authentication for API requests" for everyone. If this has broken something, please contact bugzilla-admin@mozilla.org
Last Comment Bug 947233 - Assertion failure: !cx->isExceptionPending(), at ../jsapi.cpp:2526 due to OOM in NameResolver
: Assertion failure: !cx->isExceptionPending(), at ../jsapi.cpp:2526 due to OOM...
Status: RESOLVED FIXED
: assertion, testcase
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86_64 Linux
: -- critical (vote)
: mozilla29
Assigned To: Christian Holler (:decoder)
: general
: Jason Orendorff [:jorendorff]
Mentors:
: 936971 (view as bug list)
Depends on:
Blocks: langfuzz 912928 936971
  Show dependency treegraph
 
Reported: 2013-12-06 07:10 PST by Christian Holler (:decoder)
Modified: 2013-12-09 14:26 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
js-resolveFun-oom.patch (8.73 KB, patch)
2013-12-06 07:10 PST, Christian Holler (:decoder)
no flags Details | Diff | Splinter Review
js-resolveFun-oom.patch (8.09 KB, patch)
2013-12-06 15:54 PST, Christian Holler (:decoder)
no flags Details | Diff | Splinter Review
js-resolveFun-oom.patch (8.08 KB, patch)
2013-12-06 16:52 PST, Christian Holler (:decoder)
jimb: review+
Details | Diff | Splinter Review

Description User image Christian Holler (:decoder) 2013-12-06 07:10:10 PST
Created attachment 8343752 [details] [diff] [review]
js-resolveFun-oom.patch

Hitting this assertion from time to time while fuzzing, with no good tests available. I've analyzed the problem and it seems that in [@ resolveFun] can fail due to OOM (and will return NULL in that case), but also returns NULL in other non-failure cases.

The attached patch

* refactors [@ resolveFun] to return bool to indicate success instead of returning the pointer directly. I've converted all returns to true except the one failing to due OOM. One or the other returns could also be hit by an OOM path but I'm not sure about that, so I'm leaving them alone for now.

* refactors [@ resolve] to return bool to propagate the issue, as jimb requested. 
* passes all jit-tests
* fixes the assertion (confirmed on a fuzzing box).

Feedback welcome :)
Comment 1 User image Christian Holler (:decoder) 2013-12-06 07:50:43 PST
*** Bug 936971 has been marked as a duplicate of this bug. ***
Comment 2 User image Jim Blandy :jimb 2013-12-06 15:03:18 PST
Comment on attachment 8343752 [details] [diff] [review]
js-resolveFun-oom.patch

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

I don't think this is right. resolveFun should return true on success, false on OOM. You've got it returning true on OOM, in many cases; I've pointed out a few below, but there are others. Also, when we do get an OOM, we probably don't need to bother assigning anything to *retAtom.

The changes to 'resolve' all look right, though.

::: js/src/frontend/NameFunctions.cpp
@@ +189,4 @@
>              if (!buf.append(prefix) ||
>                  !buf.append("/") ||
>                  !buf.append(fun->displayAtom()))
> +                return true;

Here's one that seems to me it should be 'return false'.

@@ +196,5 @@
>  
>          /* If a prefix is specified, then it is a form of namespace */
> +        if (prefix != nullptr && (!buf.append(prefix) || !buf.append("/"))) {
> +            *retAtom = nullptr;
> +            return true;

Here's another one which looks like an OOM path to me.
Comment 3 User image Christian Holler (:decoder) 2013-12-06 15:54:30 PST
Created attachment 8344063 [details] [diff] [review]
js-resolveFun-oom.patch

Patch v2, review comments addressed as discussed on IRC.
Comment 4 User image Jim Blandy :jimb 2013-12-06 16:06:19 PST
Comment on attachment 8344063 [details] [diff] [review]
js-resolveFun-oom.patch

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

Looks much better. Still one fix needed. Also a suggestion.

::: js/src/frontend/NameFunctions.cpp
@@ +172,5 @@
>       * Resolve the name of a function. If the function already has a name
>       * listed, then it is skipped. Otherwise an intelligent name is guessed to
>       * assign to the function's displayAtom field
>       */
> +    bool resolveFun(ParseNode *pn, HandleAtom prefix, JSAtom** retAtom) {

Could we use a MutableHandleAtom for 'retAtom' here, with the appropriate changes to the caller?

@@ +245,5 @@
>           * other namespace are rather considered as "contributing" to the outer
>           * function, so give them a contribution symbol here.
>           */
> +        if ((!buf.empty() && *(buf.end() - 1) == '/' && !buf.append("<")) || buf.empty())
> +            return false;

You merged two 'return nullptr' statements here. I think the first was an OOM return, so 'return false' is right.

But the second, where buf.empty() is true, was a "we have no name" return - which should be a 'return true' with *retAtom == nullptr.

@@ +281,5 @@
> +            JSAtom* resolvedFun = nullptr;
> +            if (!resolveFun(cur, prefix, &resolvedFun))
> +                return false;
> +
> +            RootedAtom prefix2(cx, resolvedFun);

As suggested above: let's construct the RootedAtom first, and then pass it to resolvedFun as a MutableHandleAtom. Should be as simple as passing '&prefix2'; MutableHandle<T> can be constructed from a Rooted<T> *.
Comment 5 User image Christian Holler (:decoder) 2013-12-06 16:52:56 PST
Created attachment 8344083 [details] [diff] [review]
js-resolveFun-oom.patch

Is this how you suggested it? :)
Comment 6 User image Jim Blandy :jimb 2013-12-06 16:57:12 PST
Comment on attachment 8344083 [details] [diff] [review]
js-resolveFun-oom.patch

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

Looks great. Just one thing that needs to be fixed, and this can land.

::: js/src/frontend/NameFunctions.cpp
@@ +244,5 @@
>           * functions which are "genuinely anonymous" but are contained in some
>           * other namespace are rather considered as "contributing" to the outer
>           * function, so give them a contribution symbol here.
>           */
> +        if ((!buf.empty() && *(buf.end() - 1) == '/' && !buf.append("<")) || buf.empty())

Don't forget to take out that '|| buf.empty()'! :)

@@ +282,3 @@
>  
>          if (cur->isKind(PNK_FUNCTION) && cur->isArity(PN_CODE)) {
> +            RootedAtom prefix2(cx, nullptr);

If you just leave off the ', nullptr', that's what the constructor will use by default.
Comment 7 User image Christian Holler (:decoder) 2013-12-09 04:42:04 PST
Thanks :) Made the changes and it's green on try. Landed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/79ae2a4a2e1e
Comment 8 User image Ryan VanderMeulen [:RyanVM] 2013-12-09 14:26:21 PST
https://hg.mozilla.org/mozilla-central/rev/79ae2a4a2e1e

Note You need to log in before you can comment on or make changes to this bug.