Last Comment Bug 723510 - Workers: double-error reporting in location.toString and incorrect assumption about JS_GetInstancePrivate
: Workers: double-error reporting in location.toString and incorrect assumption...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla13
Assigned To: Igor Bukanov
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-02-02 07:13 PST by Igor Bukanov
Modified: 2012-02-04 02:34 PST (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1 (21.67 KB, patch)
2012-02-03 04:27 PST, Igor Bukanov
bent.mozilla: review+
Details | Diff | Splinter Review

Description Igor Bukanov 2012-02-02 07:13:36 PST
ToString from https://hg.mozilla.org/mozilla-central/file/005980552224/dom/workers/Location.cpp#l147 contains:

    JSObject* obj = JS_THIS_OBJECT(aCx, aVp);

    JSClass* classPtr;
    if (!obj || ((classPtr = JS_GET_CLASS(aCx, obj)) != &sClass)) {
      JS_ReportErrorNumber(aCx, js_GetErrorMessage, NULL,
                           JSMSG_INCOMPATIBLE_PROTO, sClass.name, "toString",
                           classPtr ? classPtr->name : "object");
      return false;
    }

As JS_THIS_OBJECT reports errors (OOM) on its own, reporting JSMSG_INCOMPATIBLE_PROTO again is wrong after JS_THIS_OBJECT fails.

Another issue that I sported when looking for a similar pattern is the following comment in many GetInstancePrivate implementations:

    // JS_GetInstancePrivate is ok to be called with a null aObj, so this should
    // be too.

The implication is the comment is wrong. JS_GetInstancePrivate allows null aObj only for compatibility. The new code should never pass null there. As in all cases workers' GetInstancePrivate are not used with null aObj, for code simplicity the implementation should assume that aObj is not null.
Comment 1 Igor Bukanov 2012-02-03 04:27:42 PST
Created attachment 594125 [details] [diff] [review]
v1

The patch fixes ToString implementation and changes GetInstancePrivate to assume that obj is not null. The patch also removed few checks for null result of JS_GET_CLASS(obj). That is not possible.
Comment 2 Ben Turner (not reading bugmail, use the needinfo flag!) 2012-02-03 08:51:51 PST
Comment on attachment 594125 [details] [diff] [review]
v1

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

Thanks for cleaning this up!

::: dom/workers/Location.cpp
@@ +148,5 @@
>    ToString(JSContext* aCx, uintN aArgc, jsval* aVp)
>    {
>      JSObject* obj = JS_THIS_OBJECT(aCx, aVp);
> +    if (!obj)
> +      return false;

Nit: Please add braces.
Comment 4 Marco Bonardo [::mak] (Away 6-20 Aug) 2012-02-04 02:34:13 PST
https://hg.mozilla.org/mozilla-central/rev/fbd68d8e15b9

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