Closed Bug 691746 Opened 8 years ago Closed 8 years ago

Assertion failure: JSID_IS_STRING(id) || JSID_IS_INT(id), at jswatchpoint.cpp:207

Categories

(Core :: JavaScript Engine, defect, critical)

x86_64
Linux
defect
Not set
critical

Tracking

()

VERIFIED FIXED
Tracking Status
firefox7 - ---
firefox8 - wontfix
firefox9 + fixed
firefox10 + fixed
status1.9.2 --- unaffected

People

(Reporter: decoder, Assigned: jorendorff)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, testcase, Whiteboard: [sg:critical][qa-])

Attachments

(1 file)

The following test asserts on mozilla-central revision a896a9e237a0 (options -m -n):


var obj = {};
obj.watch(QName(), function () {});
gc();


S-s because this is a gc related assert. It does not crash though in optimized builds. The original test was much more complex due to the missing gc() call, which I added manually.
We're crashing in watchpoint code. Not sure what the intent is, it may as simple as just not allowing XML names as keys for Object.watch. Jason, could you take a look at this?
Assignee: general → jorendorff
Whiteboard: js-triage-needed → [sg:critical?] js-triage-needed
If this is something that we can fix safely for 8 then I'd love to do that, that is, assuming that 8 is affected by this. We don't have a whole lot of time left for 8 though...
Attached patch v1Splinter Review
Attachment #568771 - Flags: review?
Given how safe this patch looks I think we can attempt to take this for 8 still.
Attachment #568771 - Flags: review? → review?(jimb)
Attachment #568771 - Flags: review?(jimb) → review+
https://hg.mozilla.org/mozilla-central/rev/58563c49a531
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Is this patch good for 8 and 9? If so, please request approvals. We're running very very low on time for 8, so it might be too late there, but the patch does look safe.
Attachment #568771 - Flags: approval-mozilla-beta?
Attachment #568771 - Flags: approval-mozilla-aurora?
Comment on attachment 568771 [details] [diff] [review]
v1

[triage comment]

Approving for 9aurora. At this point not worth the risk on beta. This is internally found so we are comfortable deferring 6 weeks.
Attachment #568771 - Flags: approval-mozilla-beta?
Attachment #568771 - Flags: approval-mozilla-beta-
Attachment #568771 - Flags: approval-mozilla-aurora?
Attachment #568771 - Flags: approval-mozilla-aurora+
But please do land on aurora this week so it makes it into Fx9 beta.

Christian: does this affect the 1.9.2 branch?
Whiteboard: [sg:critical?] js-triage-needed → [sg:critical]
Adding [qa-] to demote this for bug verification. This isn't easily verifiable by our team.
Whiteboard: [sg:critical] → [sg:critical][qa-]
(In reply to juan becerra [:juanb] from comment #10)
> Adding [qa-] to demote this for bug verification. This isn't easily
> verifiable by our team.

There is a three line JS test for this in comment 0, isn't that sufficient for verifying?
decoder, that would not be a useful kind of verification, because that is already checked in as an automated test. It runs on every build.

It's good to have both QA humans and QA machines. It's also good for the QA humans to focus on things the QA machines aren't already doing.
(In reply to Jason Orendorff [:jorendorff] from comment #12)
> decoder, that would not be a useful kind of verification, because that is
> already checked in as an automated test. It runs on every build.

Is it? I had at least one case where the test was not added to the testsuite, the bugfix was accidentially reverted and the browser shipped without the fix. Maybe QA could ensure that a test was added, and if that's the case, they don't need to do anything else. Does that make sense?
(In reply to Christian Holler (:decoder) from comment #13)
> (In reply to Jason Orendorff [:jorendorff] from comment #12)
> > decoder, that would not be a useful kind of verification, because that is
> > already checked in as an automated test. It runs on every build.
> 
> Is it?

Yes.

> I had at least one case where the test was not added to the
> testsuite, the bugfix was accidentially reverted and the browser shipped
> without the fix. Maybe QA could ensure that a test was added, and if that's
> the case, they don't need to do anything else. Does that make sense?

That sounds like a terrible use of a human being's time.
(In reply to Jason Orendorff [:jorendorff] from comment #14)
>
> That sounds like a terrible use of a human being's time.

I agree that we should automate what we can automate. To verify that the test was added to the testsuite, we will likely have to change something in the process though (I could imagine adding a testcase field to bugzilla and/or being able to mark attachments as testcases because that would allow automated scripts to check if that test was added to some testsuite).

My initial comment also wasn't meant as a recommendation to do all this manually. I was just trying to point out that "not easy to verify" vs. "automated test available in comment" does not make sense to me.
Christian, I think I overlooked comment #0. Today I was on a rush to triage a list of bugs that needed verification by human eyes, and I didn't read this well enough. I was trying to prioritize.

We could mark this as verified since there is a test included, and it runs in every build.
Sounds good to me :) Thanks for verifying! :)
qawanted: need to test whether this affects 1.9.2, from the patch it might be old code.
blocking1.9.2: --- → ?
Keywords: qawanted
Dan, you were going to look at this in a debug build?
Only the first part of this patch would apply to the 1.9.2 branch (jswatchpoint.cpp doesn't exist there, nor the watch() method). Testcase doesn't seem to be causing any issues on the old branch so we can pass on this one.
blocking1.9.2: ? → ---
Keywords: qawanted
Group: core-security
Status: RESOLVED → VERIFIED
A testcase for this bug was automatically identified at js/src/tests/js1_8_5/extensions/regress-691746.js.
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.