604381 - Panorama stops working, if set javascript.options.methodjit.chrome to true

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P2)

Description

[Alice0775 White]

Build Identifier: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b8pre) Gecko/20101014 Firefox/4.0b8pre ID:20101014041748

Panorama stops working, if set javascript.options.methodjit.chrome to true

Reproducible: Always

Steps to Reproduce:
1. Start Minefield with new profile+ javascript.options.methodjit.chrome=true
2. Open Tabs
3. Open Panorama Ctrl + E

Actual Results:
 no tab in panorama


Regression window:
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=3c762301b719&tochange=29c228a4d7eb

Comment 1

[Csaba Kozák [:WonderCsabo]]

Confirmed.

Comment 3

[Dietrich Ayala (:dietrich)]

I reported same bug for linux, and Ben for Mac, so OS -> All.

Comment 4

[Ian Gilman [:iangilman]]

It seems to have appeared between the 10/12 nightly and the 10/13 nightly, which should be this window:

http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=2593c8c8af8b&tochange=f6e81dd5a125

Comment 5

[Ian Gilman [:iangilman]]

Is this a JavaScript bug or a Tab Candy bug? Who should be working on it?

Comment 7

[Aza Raskin [:aza]]

Do we know if we will be turning this setting (javascript.options.methodjit.chrome to true) on for Firefox 4. My understanding is that we aren't, which means this bug probably doesn't need to be fixed as a P1 or P2 for shipping.

CC'ing Dave for insight.

Comment 8

[Dave Herman [:dherman]]

I'm not the Dave you're looking for. Luckily one of them (dmandelin) is CC'ed. CC'in another (dvander).

Dave #24601

Comment 9

[David Anderson [:dvander] - inactive, e-mail if emergency]

I haven't heard of any plans to turn on chrome for the method JIT, but if there's a correctness issue, it's worth diagnosing since it could affect content too.

Comment 10

[David Mandelin [:dmandelin]]

(In reply to comment #7)
> Do we know if we will be turning this setting
> (javascript.options.methodjit.chrome to true) on for Firefox 4. My
> understanding is that we aren't, which means this bug probably doesn't need to
> be fixed as a P1 or P2 for shipping.

We're still thinking about that, but leaning no per dvander in comment 9. As far as we can tell, turning on jits for chrome has no effect on subjective performance. The minus to turning it on is the extra bugs, like this one. The pluses are that bugs discovered that way probably affect content as well, and would improve the overall engine once fixed; and that some users will turn it on anyway, and we want things to work well for them too.

My personal opinion at this point is that the new jit is in a fairly early stage, and we have enough bugs to keep us busy on the content side. Once that stabilizes, whenever that is, may be the time to turn it on for chrome as well.

Comment 11

[Aza Raskin [:aza]]

Thanks for the info. In that case, it sounds like this is not something we will look at or focus on for the Firefox 4 timeline.

Comment 12

[Ian Gilman [:iangilman]]

Making this not block our beta 8 bug. 

When we do feel the JIT is ready, it may very well make a speed difference for Panorama, as it's very JavaScript-heavy. Could be a good test-case for the performance improvement of the JIT in chrome.

Comment 13

[David Mandelin [:dmandelin]]

(In reply to comment #12)
> Making this not block our beta 8 bug. 
> 
> When we do feel the JIT is ready, it may very well make a speed difference for
> Panorama, as it's very JavaScript-heavy. Could be a good test-case for the
> performance improvement of the JIT in chrome.

Interesting. Feel free to try it out any time and report how it does--I know some of our testers run with methodjit on for chrome, and it seems to work for the most part.

Comment 14

[Brendan Eich [:brendan]]

Yeah, at least keep testing -- best case, the crashes go away. Less likely but could be a lifesaver if content hits the same bug, we get some crucial diagnostic information. Please leave any such skidmarks here if you get them.

/be

Comment 15

[Csaba Kozák [:WonderCsabo]]

I use JM for chrome. I saw no improvement with Panorama compared to TM only. :(

Comment 18

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: fix

This affects the web but it's pretty obscure - you need to have a constructor that, inside a catch block, returns an object other than |this|. In chrome it's easier because it can also trigger with "let".

The bug is that constructors weren't checking fp->rval before assuming an implicit return (RETRVAL, STOP) are returning undefined.

Comment 19

[David Anderson [:dvander] - inactive, e-mail if emergency]

http://hg.mozilla.org/tracemonkey/rev/44573d17ec8c

Comment 20

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/44573d17ec8c