Closed Bug 723894 Opened 12 years ago Closed 12 years ago

Firefox 13.0a1 Crash @ JSAutoEnterCompartment::enter


(Core :: XPConnect, defect)

12 Branch
Not set



Tracking Status
firefox11 --- unaffected
firefox12 + verified


(Reporter: emk, Assigned: emk)



(Keywords: crash, regression, reproducible, Whiteboard: [qa+])

Crash Data


(1 file, 1 obsolete file)

Attached patch patch (obsolete) — Splinter Review
JSVAL_IS_OBJECT(v) is not equivalent to v.isObject(), but is equivalent to v.isObjectOrNull().
Assignee: nobody → VYV03354
Attachment #594131 - Flags: review?(mrbkap)
Severity: normal → critical
Crash Signature: [@ JSAutoEnterCompartment::enter(JSContext*, JSObject*)] [@ JSAutoEnterCompartment::enter]
Summary: Firefox 13.0a1 Crash Report [@ JSAutoEnterCompartment::enter(JSContext*, JSObject*) ] → Firefox 13.0a1 Crash @ JSAutoEnterCompartment::enter
Version: unspecified → 12 Branch
Comment on attachment 594131 [details] [diff] [review]

Review of attachment 594131 [details] [diff] [review]:

This looks pretty good -- can you add a crashtest for this as well as addressing the comment below? r- for now, but I'm sure r+ on the next patch.

::: js/xpconnect/src/
@@ +381,5 @@
>               "  if (!aCx || !aVal) {\n"
>               "    return NS_OK;\n"
>               "  }\n"
> +             "  NS_ENSURE_STATE(aVal->isObject());\n\n"
> +             "  JSObject& obj = aVal->toObject();\n"

I'd rather see this as |JSObject *obj = &aValue->toObject();| that way you can reduce the changes later and to match other toObject() using code in the tree.
Attachment #594131 - Flags: review?(mrbkap) → review-
Attached patch patch v2Splinter Review
Resolved review comments.
Attachment #594131 - Attachment is obsolete: true
Attachment #594432 - Flags: review?(mrbkap)
Blocks: 721569
Comment on attachment 594432 [details] [diff] [review]
patch v2

Perfect, thanks.
Attachment #594432 - Flags: review?(mrbkap) → review+
Keywords: checkin-needed
Whiteboard: [autoland-try]
Whiteboard: [autoland-try] → [autoland-in-queue]
Autoland Patchset:
	Patches: 594432
	Branch: mozilla-central => try
Try run started, revision d1e88e9e3e63. To cancel or monitor the job, see:
Try run for d1e88e9e3e63 is complete.
Detailed breakdown of the results available here:
Results (out of 208 total builds):
    success: 190
    warnings: 18
Builds (or logs if builds failed) available at:
Whiteboard: [autoland-in-queue]
Closed: 12 years ago
Resolution: --- → FIXED
Recent regression that will likely spike as our testing audience grows. If considered low risk, we'd definitely consider uplifting a fix if nominated.
Comment on attachment 594432 [details] [diff] [review]
patch v2

[Approval Request Comment]
Regression caused by (bug #): 709569
User impact if declined: DoS attack can be made because this is a reproducible crash.
Testing completed (on m-c, etc.): Crashtest added on m-c.
Risk to taking this patch (and alternatives if risky): Low, trivial one line fix.
String changes made by this patch: No.
Attachment #594432 - Flags: approval-mozilla-aurora?
Comment on attachment 594432 [details] [diff] [review]
patch v2

[Triage Comment]
Approved for Aurora 12.
Attachment #594432 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Keywords: checkin-needed
Whiteboard: [land to aurora]
Whiteboard: [qa+]
Mozilla/5.0 (X11; Linux i686; rv:12.0) Gecko/20100101 Firefox/12.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:12.0) Gecko/20100101 Firefox/12.0
Mozilla/5.0 (Windows NT 5.1; rv:12.0) Gecko/20100101 Firefox/12.0

Verified in Firefox 12 beta3 on Windows XP, Mac OS 10.6, ubuntu 11.10.

No crash when loading new MouseEvent("click", null); in Error Console. Could previously reproduce.
You need to log in before you can comment on or make changes to this bug.