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

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

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.

Comment 1

[David Mandelin [:dmandelin]]

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?

Comment 2

[Johnny Stenback (:jst)]

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...

Comment 3

[Jason Orendorff [:jorendorff]]

Attachment: v1

Comment 4

[Johnny Stenback (:jst)]

Given how safe this patch looks I think we can attempt to take this for 8 still.

Comment 5

[Jason Orendorff [:jorendorff]]

https://hg.mozilla.org/mozilla-central/rev/58563c49a531

Comment 6

[Johnny Stenback (:jst)]

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.

Comment 7

[LegNeato]

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.

Comment 8

[Daniel Veditz [:dveditz]]

But please do land on aurora this week so it makes it into Fx9 beta.

Christian: does this affect the 1.9.2 branch?

Comment 9

[David Mandelin [:dmandelin]]

http://hg.mozilla.org/releases/mozilla-aurora/rev/c8fcdb6bd4d7

Comment 10

[juan becerra [:juanb]]

Adding [qa-] to demote this for bug verification. This isn't easily verifiable by our team.

Comment 11

[Christian Holler (:decoder)]

(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?

Comment 12

[Jason Orendorff [:jorendorff]]

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.

Comment 13

[Christian Holler (:decoder)]

(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?

Comment 14

[Jason Orendorff [:jorendorff]]

(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.

Comment 15

[Christian Holler (:decoder)]

(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.

Comment 16

[juan becerra [:juanb]]

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.

Comment 17

[Christian Holler (:decoder)]

Sounds good to me :) Thanks for verifying! :)

Comment 18

[Daniel Veditz [:dveditz]]

qawanted: need to test whether this affects 1.9.2, from the patch it might be old code.

Comment 19

[Al Billings [:abillings - ex-MoCo]]

Dan, you were going to look at this in a debug build?

Comment 20

[Daniel Veditz [:dveditz]]

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.

Comment 21

[Christian Holler (:decoder)]

A testcase for this bug was automatically identified at js/src/tests/js1_8_5/extensions/regress-691746.js.