488015 - Crash [@ js_GetUpvar ] (also bogus JS errors, also probably Crash [@js_Interpret])

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Hideo Oshima]

Steps to reproduce:
1. Open http://ytooyama.spaces.live.com/
2. Click anywhere on the content area or close tab / window

Crash report is http://crash-stats.mozilla.com/report/index/9395c260-acc4-4347-a1e6-1f9332090412 .

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2a1pre) Gecko/20090411 Minefield/3.6a1pre

Comment 1

[Atsushi Sakai]

On Mac, with mozilla-central,
20090406 works fine
20090407 Crash [@ js_Interpret ]

20090409 Crash [@ js_Interpret ]
20090410 Crash [@ js_GetUpvar ]

Comment 2

[Brendan Eich [:brendan]]

Dup of bug 488034?

/be

Comment 3

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

The URL is crashing from running the JS file here (in a <script></script> tag):

http://js.wlxrs.com/WZd8QQx!qfUOCd3U!RJ6!g/liveframeworkex.js

However, this 80,514 bytes JS file is so complex for reduction, I am taking quite a bit of time reducing it.

Comment 4

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: Half-sized testcase (700+ lines)

Here's a 700+ line testcase that crashes latest mozilla-central on Mac Leopard. Open it and left click once in the content area to crash.

(Took me the better part of an hour just to get it down by half from around 1,400 to 1,500 lines, after adding and tweaking arbitrary newlines in the guilty js file above, which previous was merely a very long one-liner.)

Comment 5

[Atsushi Sakai]

(In reply to comment #2)
> Dup of bug 488034?

Those seems different.

This bug: In js_GetUpvar(), fp is NULL.
Bug 488034: In js_GetUpvar(), fp is not NULL, but can't be read at its address.

Comment 6

[Atsushi Sakai]

Attachment: reduced testcase

Crashes on mousedown.

Comment 7

[Brendan Eich [:brendan]]

Attachment: js shell version of html testcase

Thanks so much for the reduced testcase!

/be

Comment 8

[Brendan Eich [:brendan]]

The top-level var b that the nested function b shadows is required for the bug to bite.

/be

Comment 9

[Jason Orendorff [:jorendorff]]

This gets compiled correctly:

    var b;
    (function () {
      function b() {}                        // [1]
      print(dis(function() { b(); }));       // [2]
    })();

    flags: LAMBDA FLAT_CLOSURE
    main:
    00000:  calldslot 0
    00003:  call 0
    00006:  pop
    00007:  stop

But after switching two lines:

    var b;
    (function () {
      print(dis(function() { b(); }));       // [2]
      function b() {}                        // [1]
    })();

    flags: LAMBDA NULL_CLOSURE
    main:
    00000:  callname "b"
    00003:  call 0
    00006:  pop
    00007:  stop

I'm guessing that CALLNAME isn't intended to work in cases like this where the name really should be statically bound.

Comment 10

[Jason Orendorff [:jorendorff]]

Comment 9 in the form of a test.

function b() { throw "FAIL"; }
(function () {
  (function() { b(); })();
  function b() { print("PASS"); }
})();

Comment 11

[Brendan Eich [:brendan]]

Yes, this is a bug in the bottom-up analysis. Have to reorganize it a bit...

/be

Comment 12

[Brendan Eich [:brendan]]

I saw problems probably due to this bug on live maps last night. This bug has potentially many symptoms, not all crashers, as the reduced test demonstrates. Link potential dups as dependencies, we'll have to see which ones are fixed by the patch I will put up today, if we can't reduce and prove they are dups.

/be

Comment 13

[Brendan Eich [:brendan]]

Attachment: fix

Greatly simplified and corrected at the same time. The JSTreeContext.upvars list was a bad idea, it captured used after defined upvars but missed defined after used upvars that shadowed defined before apparently used (in source order) outer upvars, as in the minimal testcase for this bug.

As always, generator expressions require extra work.

/be

Comment 15

[Brendan Eich [:brendan]]

Attachment: rebased to tm tip

I'm going to check this into tm ahead of mrbkap-hockey's review, to get some builds people can test. Will back out or amend as needed.

/be

Comment 16

[Brendan Eich [:brendan]]

http://hg.mozilla.org/tracemonkey/rev/78a21b8efe1b

/be

Comment 17

[Brendan Eich [:brendan]]

(In reply to comment #13)
> Created an attachment (id=372797) [details]
> fix
> 
> Greatly simplified and corrected at the same time. The JSTreeContext.upvars
> list was a bad idea, it captured used after defined upvars but missed defined
> after used upvars that shadowed defined before apparently used (in source
> order) outer upvars, as in the minimal testcase for this bug.

Folks following closely will see that before this bug's patch, any function using upvars only one level up, in an enclosing function, was fine. Same for n > 1 levels up provided no m < n upvar shadowed the outer level-n upvar after the innermost function was analyzed.

It's amazing how much code worked. Shallow is better than deep.

/be

Comment 18

[Mike Beltzner [:beltzner, not reading bugmail]]

(Is there someone other than bkap who can take this review? He's swamped under the weight of another P1 blocker at the moment ...)

Comment 20

[Brendan Eich [:brendan]]

(In reply to comment #18)

mrbkap'll do it fastest. At this point testing (Gary's fuzzing, others running tm builds) is more imp.

The code now matches my original pseudo-code in the big comment in jsparse.h, the one just before JSDefinition's declaration. The tc->upvars bum steer came from a little red guy on my shoulder... all is well now.

/be

Comment 21

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/78a21b8efe1b

Comment 23

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/ec54f7218d9a

Comment 24

[Marcia Knous [:marcia]]

Verified fixed on the 1.9.1 branch using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b4pre) Gecko/20090420 Shiretoko/3.5b4pre and Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b4pre) Gecko/20090420 Shiretoko/3.5b4pre.

Verified fixed on the trunk using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090420 Minefield/3.6a1pre and Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090420 Minefield/3.6a1pre.

Comment 25

[Gabriela [:gaby2300]]

(In reply to comment #0)
> Steps to reproduce:
> 1. Open http://ytooyama.spaces.live.com/
> 2. Click anywhere on the content area or close tab / window
> 
> Crash report is
> http://crash-stats.mozilla.com/report/index/9395c260-acc4-4347-a1e6-1f9332090412
> .
> 
> Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2a1pre) Gecko/20090411
> Minefield/3.6a1pre

As of today (April 22 23:30 GMT) I'm having no trouble at all with the site

Comment 26

[klonos]

never tested the testcase to see if I actually crash, but I verify that I don't crash with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090422 Minefield/3.6a1pre (.NET CLR 3.5.30729) ID:20090422044118

Comment 27

[Christian Holler (:decoder)]

Automatically extracted testcase for this bug was committed:

https://hg.mozilla.org/mozilla-central/rev/efaf8960a929