361964 - Crash [@ MarkGCThingChildren] involving watch and setter

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[moz_bug_r_a4]

Crash:
  document.watch("title", function(a,b,c,d) {
    return { toString : function() { alert(1); } };
  });
  document.title = "xxx";

No crash:
  document.watch("title", function() {
    return { toString : function() { alert(1); } };
  });
  document.title = "xxx";

Regression range on trunk is from 2006-11-06-04 to 2006-11-07-04.
http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&date=explicit&mindate=2006-11-06+04&maxdate=2006-11-07+04

Regression range on 1.8.0 branch is from 2006-11-24-07 to 2006-11-25-08
http://bonsai.mozilla.org/cvsquery.cgi?module=AviarySuiteBranchTinderbox&branch=MOZILLA_1_8_0_BRANCH&date=explicit&mindate=2006-11-24+07&maxdate=2006-11-25+08

Regressed from bug 354978?

I'm marking this security sensitive for now, since I'm not sure if this could
be an exploitable crash or not.

Steps to reproduce:
0. Load a testcase.
1. Click a button.
2. An alert dialog appears.
3. Don't close the alert dialog.
4. After a few seconds, crash.

Comment 1

[moz_bug_r_a4]

Attachment: testcase

Comment 2

[moz_bug_r_a4]

Attachment: backtrace from debug build

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Possible fix

I haven't actually tested this, but I think the problem is that we're trying to mark fun->nargs + fun->extra slots, when we're only providing two memory spaces.

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 246687 [details] [diff] [review]
Possible fix

>+                frame.scopeChain = OBJ_GET_PARENT(cx, closure);

This line is not actually part of this patch.

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Updated

Comment 6

[Brendan Eich [:brendan]]

Comment on attachment 246733 [details] [diff] [review]
Updated

r=me, thanks.

/be

Comment 7

[Brendan Eich [:brendan]]

Need this on the trunk when it opens, and on the branches sooner.

/be

Comment 8

[Jay Patel [:jay]]

mrbkap/brendan:  Is the patch good as-is for the branches?  If so, please ask for approval for 1.8.0.9 and 1.8.1.1.  If we need separate patches, please attach and ask for separate approvals.  Thanks!

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

Fixed on trunk.

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 246733 [details] [diff] [review]
Updated

This patch applies cleanly to the 1.8 branch.

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 246733 [details] [diff] [review]
Updated

This patch applies cleanly to the 1.8 branch.

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Patch for 1.8.0

The only changes are:
- fun->u.n.extra -> fun->extra (since the unionizing of JSFunction didn't happen on the 1.8.0 branch).
- JS_ARRAY_LENGTH doesn't exist on the 1.8.0 branch, so I hand-expanded it.

I don't think this needs another review.

Comment 13

[Jay Patel [:jay]]

Comment on attachment 246733 [details] [diff] [review]
Updated

Approved for 1.8 branch, a=jay for drivers.

Comment 14

[Jay Patel [:jay]]

Comment on attachment 247101 [details] [diff] [review]
Patch for 1.8.0

Approved for 1.8.0 branch, a=jay for drivers.

Comment 15

[Blake Kaplan (:mrbkap) (inactive)]

Fixed on the 1.8 branches.

Comment 16

[Bob Clary [:bc] (inactive)]

Attachment: js1_5/Regress/regress-361964.js

Comment 17

[moz_bug_r_a4]

I can still reproduce crash.

This seems to be due to missing |* sizeof(jsval)|.

- argv = JS_malloc(cx, nslots);
+ argv = JS_malloc(cx, nslots * sizeof(jsval));

- memset(argv + 2, 0, nslots - 2);
+ memset(argv + 2, 0, (nslots - 2) * sizeof(jsval));

Comment 18

[Bob Clary [:bc] (inactive)]

In trunk winxp debug I get MSVC CRT Debug error: Heap Corruption Detected... detected that the application wrote to memory after the end of the heap buffer. 

In 1.8.0.9|1.8.1.1 winxp debug I get MSCC CRT Debug error: DAMAGE after normal block

This should block the Firefox 1.5.0.9, Firefox 2.0.0.1 and Gran Paradiso releases.

Comment 19

[Jay Patel [:jay]]

Removing "fixed1.8.x.x" keywords since this doesn't appear to be fixed.  

Blake/Brendan:  We are past our code freeze date, but really need to get this fixed, so please let us know if either of you can take a look at this.  It would be nice to have a patch together by Monday morning, since we were hoping to spin RC1 then.

Comment 20

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: D'oh

I blame all of the Java code I've had to write lately.

Comment 21

[Brendan Eich [:brendan]]

Comment on attachment 247282 [details] [diff] [review]
D'oh

I have no Java-on-the-brainn excuse :-(.

This needs prompt approval for the branches.

/be

Comment 22

[Blake Kaplan (:mrbkap) (inactive)]

Fixed (again) on trunk.

Comment 23

[Jay Patel [:jay]]

Comment on attachment 247282 [details] [diff] [review]
D'oh

Approved for both branches, a=jay for drivers.  Thanks for the quick patch patch mrbkap. ;-)

Comment 24

[Blake Kaplan (:mrbkap) (inactive)]

Fixed, again.

Thanks for the heads-up moz_bug.

Comment 25

[Bob Clary [:bc] (inactive)]

verified fixed 20061203 1.8.0.9 windows/linux, 1.8.1.1 windows/linux, 1.9 windows/linux

Comment 26

[Bob Clary [:bc] (inactive)]

/cvsroot/mozilla/js/tests/js1_5/extensions/regress-361964.js,v  <--  regress-361964.js