Closed Bug 488015 Opened 11 years ago Closed 11 years ago

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

Categories

(Core :: JavaScript Engine, defect, P1, critical)

defect

Tracking

()

VERIFIED FIXED
mozilla1.9.2a1

People

(Reporter: hidenosuke, Assigned: brendan)

References

()

Details

(4 keywords, Whiteboard: fixed-in-tracemonkey)

Crash Data

Attachments

(4 files, 1 obsolete file)

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
On Mac, with mozilla-central,
20090406 works fine
20090407 Crash [@ js_Interpret ]

20090409 Crash [@ js_Interpret ]
20090410 Crash [@ js_GetUpvar ]
Assignee: nobody → general
Blocks: upvar2
Severity: normal → critical
Component: General → JavaScript Engine
Keywords: crash, regression
OS: Linux → All
Product: Firefox → Core
QA Contact: general → general
Version: unspecified → Trunk
Flags: blocking1.9.1?
Dup of bug 488034?

/be
Depends on: 488034
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.
Hardware: x86 → All
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.)
(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.
Attached file reduced testcase
Crashes on mousedown.
Flags: blocking1.9.1? → blocking1.9.1+
Priority: -- → P1
Thanks so much for the reduced testcase!

/be
Assignee: general → brendan
Status: NEW → ASSIGNED
The top-level var b that the nested function b shadows is required for the bug to bite.

/be
Flags: in-testsuite?
Target Milestone: --- → mozilla1.9.1b4
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 9 in the form of a test.

function b() { throw "FAIL"; }
(function () {
  (function() { b(); })();
  function b() { print("PASS"); }
})();
Yes, this is a bug in the bottom-up analysis. Have to reorganize it a bit...

/be
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
Blocks: 488030
Attached patch fix (obsolete) — Splinter Review
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
Attachment #372797 - Flags: review?(mrbkap)
Duplicate of this bug: 487957
Summary: Crash [@ js_GetUpvar ] → Crash [@ js_GetUpvar ] (also bogus JS errors, also probably Crash [@js_Interpret])
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
Attachment #372797 - Attachment is obsolete: true
Attachment #372830 - Flags: review?(mrbkap)
Attachment #372797 - Flags: review?(mrbkap)
http://hg.mozilla.org/tracemonkey/rev/78a21b8efe1b

/be
Whiteboard: fixed-in-tracemonkey
(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
Depends on: 488475
(Is there someone other than bkap who can take this review? He's swamped under the weight of another P1 blocker at the moment ...)
Duplicate of this bug: 488030
(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
Attachment #372830 - Flags: review?(mrbkap) → review+
http://hg.mozilla.org/mozilla-central/rev/78a21b8efe1b
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Duplicate of this bug: 488604
Depends on: 488690
Depends on: 488848
Blocks: 489034
Depends on: 489130
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.
Status: RESOLVED → VERIFIED
Target Milestone: mozilla1.9.1b4 → mozilla1.9.2a1
(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
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
Depends on: 490741
Depends on: 492673
No longer depends on: 492673
Depends on: 576847
Attachment #372361 - Attachment is patch: false
Depends on: 589101
Crash Signature: [@ js_GetUpvar ] [@js_Interpret]
Automatically extracted testcase for this bug was committed:

https://hg.mozilla.org/mozilla-central/rev/efaf8960a929
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.