406769 - simple code now causes a slow script warning dialog to appear when it didn't before (seen with All In One Gestures extension at start-up)

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Steve England [:stevee]]

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9b2pre) Gecko/2007120319 Minefield/3.0b2pre ID:2007120319

1. New profile, start firefox.
2. Set extensions.checkUpdateSecurity and extensions.checkCompatibility to false.
3. Install All-in-1 Mouse Gestures extension from https://addons.mozilla.org/en-US/firefox/downloads/file/60/all-in-one_gestures-0.18.0-fx.xpi
4. Close firefox
5. Start firefox

Expected:
- Firefox starts up unimpeded.

Actual:
- Firefox appears in the process list for a while, before a dialog appears: 

      A script on this page may be busy, or it may have stopped
      responding. You can stop the script now, or you can continue
      to see if the script will complete.

      Script: chrome://allinonegest/content/allinonegestOverlay.js:73

Works fine: 20071203_1859_firefox-3.0b2pre.en-US.win32
Odd behaviour: 20071203_1916_firefox-3.0b2pre.en-US.win32

Checkins to module PhoenixTinderbox between 2007-12-03 18:59 and 2007-12-03 19:15 : 
http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&date=explicit&mindate=1196737140&maxdate=1196738159

Due to bug 376957.

Comment 1

[Mike Shaver (:shaver emeritus)]

Sounds like All-in-1 will need to update to take into account the (interoperable, being standardized in ES4) change here before they declare themselves compatible -- is this really a Core bug?  Recommend INVALID, I think.

Comment 2

[Steve England [:stevee]]

With a new profile and All-in-1 Gestures 0.18.0 installed.. If I set javascript.options.showInConsole to true, and then start up the 20071203_1859 build I get nothing reported in the Error Console. But with the 20071203_1916 build I'm seeing:

        Error: aioPrefListener is undefined
        Source file: chrome://allinonegest/content/allinonegestOverlay.js
        Line: 116

thrown twice.

Comment 3

[Damon Sicore (:damons)]

Based on comment 1, and the fact that the extension needs to be updated, -'ing.  If more info becomes available, and you think this should block.  Please re-nom.

Comment 4

[:Gavin Sharp [email: [e-mail]]]

Why did we conclude that the extension needs updating? Did anyone actually verify that it wasn't a problem with that patch? Steve has pretty conclusive evidence that the landing of the patch for bug 376957 is what caused this, and I don't think we should dismiss it as "the extension needs to be updated" until we know that the cause of the bustage is that it relies on behavior that we've decided to change.

Comment 5

[Dão Gottwald [:dao]]

I second that this bug should be blocking in that it needs a resolution -- which can be INVALID, too.
To me it's not obvious what that extension is doing wrong. Maybe Shaver can elaborate on this.

Comment 6

[Mike Schroepfer]

Shaver?  is this a core bug or does the extension need to update...

Comment 7

[Steve England [:stevee]]

FWIW I've used All-in-1 Gestures 0.18.0 for something like 18 months on nightly trunk builds with no problems, until this happened.

Comment 8

[Mike Shaver (:shaver emeritus)]

I don't know if it's a Core bug; I don't know anything about that extension, and it's huge, so without the author's co-operation here or some other test case, I don't know how we'll get to clarity.  When I made my comment the only data I had was Steve's from comment #0, indicating that Ai1G wasn't tested to be compatible and wasn't compatible -- is the author one of the people on this bug?

Comment 9

[Steve England [:stevee]]

I've just CC'd the author.

Comment 10

[Marc Boullet]

I'm Marc Boullet, the author of AiOG extension.

I just received the following mail from Mark Tickner:
"Hi,
 
I have been running your very nice extension in the beta version of Mozilla Firefox for a while now using the Nightly Tester Tools to override compatibility.
 
The 2007-12-04-04 update seems to have broken something.  This is what I get from the Venkman debugger.
 
#0: function (null)() in <chrome://allinonegest/content/allinonegestOverlay.js> line 73
071: var aioSofar = []; aioSofar[1] = 0;
072: for (var ii = 1; ii < aioDist.length -1; ++ii)
073: aioSofar[ii+1] = aioSofar[ii] + (aioDist[ii] - aioDist[ii-1]) * aioRatio[ii];
074: const aioCursors = ["move", "n-resize", "e-resize"];
075: var aioScrollCount, aioScrollRate, aioScrollMax, aioASPeriod;
You have a really nice extension, so it's a shame something just got broken.
 
Best Regards"

The sequence of lines is :
const aioDist =  [0, 20, 40, 60, 80, 100, 130, 180, 300, 5000];
const aioRatio = [.0, .067, .083, .108, .145, .2, .3, .45, .65, .9];
const aioScrollLoop = [1, 2, 4];
var aioSofar = []; aioSofar[1] = 0;
for (var ii = 1; ii < aioDist.length -1; ++ii)
   aioSofar[ii+1] = aioSofar[ii] + (aioDist[ii] - aioDist[ii-1]) * aioRatio[ii];

Seems there is a problem on the last line but it's quite basic code and I don't see anything wrong with it. Any ideas ?

Comment 11

[:Gavin Sharp [email: [e-mail]]]

Attachment: testcase

This is a strange one. I can reproduce the "hang" with this testcase, when loaded locally (gives me the "slow script" dialog). It was reduced from the source of the All-in-One tabs extension from comment 0.

Removing a single var declaration from the source causes the problem to not appear. While reducing I noticed that I could replace, for example, |var a="boo";| with |var a; var b;| and still reproduce the problem, but if I changed |var="boo";| to just |var a;|, the problem would disappear, as though the size of the interpreted code in memory mattered. I tried looking for alternatives to the loop at the bottom, but couldn't find any. The array object in the last line also seems to be required.

Comment 12

[:Gavin Sharp [email: [e-mail]]]

The testcase seems to work when accessed via Bugzilla directly over https, as well. With a current trunk build, I get a slow script dialog, whereas with a branch build I don't (as expected for such trivial code).

Comment 13

[Mike Schroepfer]

marking blocking until we can figure out exactly what is going on.  Igor/Brendan any changes in that time range that might be relevant?

Comment 14

[Igor Bukanov]

This can be related to bug 407501.

Comment 15

[Igor Bukanov]

(In reply to comment #14)
> This can be related to bug 407501.

In fact I think this is caused by bug 407501 as the GC hazard can cause the behavior from comment 11. 

Comment 16

[Igor Bukanov]

With the patch for 407501 I still got a slow warning for the test case from comment 11. So this is something else.

Comment 17

[Igor Bukanov]

Attachment: Test case for js shell

The test case that currently goes into infinite loop with js shell.

Comment 18

[Igor Bukanov]

This is a bug somewhere in the code generator. For the test case from comment 17 the bytecode is:

00000:  defvar "a0"
...
00378:  defvar "a126"
main:
00381:  name "a0"
00384:  pop
...
00881:  name "a125"
00884:  pop
00885:  one
00886:  setgvar "a126"
00889:  pop
00890:  getgvar "a126"
00893:  newinit 3
00895:  zero
00896:  one
00897:  initelem
00898:  one
00899:  int8 2
00901:  initelem
00902:  int8 2
00904:  int8 3
00906:  initelem
00907:  endinit
00908:  group
00909:  getprop "length"
00912:  one
00913:  sub
00914:  lt
00915:  ifeq 927 (12)
00918:  one
00919:  popv
00920:  getgvar "a126"
00923:  pop
00924:  goto 890 (-34)
00927:  stop


Not that the bytecode at the position 920 is getgvar "a126", which just fetches the value for the golbal a126, not increases it as required in the for loop for the test:

for (var a126 = 1; a126 < ([1,2,3]).length -1; ++a126)
    1;

If I comment out "var a125;" in the test case, then the test terminates and the bytecode becomes:

00000:  defvar "a0"
...
00375:  defvar "a126"
main:
00378:  name "a0"
00381:  pop
...
00874:  name "a124"
00877:  pop
00878:  one
00879:  setgvar "a126"
00882:  pop
00883:  getgvar "a126"
00886:  newinit 3
00888:  zero
00889:  one
00890:  initelem
00891:  one
00892:  int8 2
00894:  initelem
00895:  int8 2
00897:  int8 3
00899:  initelem
00900:  endinit
00901:  group
00902:  getprop "length"
00905:  one
00906:  sub
00907:  lt
00908:  ifeq 920 (12)
00911:  one
00912:  popv
00913:  incgvar "a126"
00916:  pop
00917:  goto 883 (-34)
00920:  stop

with the proper invgvar at the position 913.

Comment 19

[Igor Bukanov]

This is regression caused by bug 398609. The changes from bug 376957 just exposed it. As such the patch there is innocent.

Comment 20

[Igor Bukanov]

Attachment: fix v1

In the patch for bug 398609 I have used the wrong ale to extract the constness status of the operation.

Comment 21

[Igor Bukanov]

It would be nice to fix this in b2 as it exposed a bad regression in the global variable optimizer.

Comment 22

[Igor Bukanov]

I will be away until approximately 20:00 UTC/12:00 PST. So if the patch from comment 20 would get approvals, I could land it only after that time. 

Comment 23

[:Gavin Sharp [email: [e-mail]]]

I've confirmed that that patch fixes the problem for me. Thanks Igor!

Comment 24

[Brendan Eich [:brendan]]

Comment on attachment 292302 [details] [diff] [review]
fix v1

We need to fix this for b2, so help landing today may be needed.

/be

Comment 25

[Igor Bukanov]

I am going to sleep (In reply to comment #24)
> (From update of attachment 292302 [details] [diff] [review])
> We need to fix this for b2, so help landing today may be needed.

I can not land the patch myself today - I am going to sleep soon.

Comment 26

[Mike Schroepfer]

Comment on attachment 292302 [details] [diff] [review]
fix v1

yep - we want this for b2..

Comment 27

[:Gavin Sharp [email: [e-mail]]]

I landed attachment 292302 [details] [diff] [review] on the trunk:
mozilla/js/src/jsemit.c 	3.291

I'll trigger a clobber to push out new nightlies.

Comment 28

[Steve England [:stevee]]

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9b2pre) Gecko/2007120915 Minefield/3.0b2pre ID:2007120915

AiOG 0.18 now works again
--> VERIFIED

Comment 29

[Steve England [:stevee]]

(Gavin's testcase now works too)

Comment 30

[Bob Clary [:bc] (inactive)]

Checking in js1_5/Regress/regress-406769.js;
/cvsroot/mozilla/js/tests/js1_5/Regress/regress-406769.js,v  <--  regress-406769.js
initial revision: 1.1
done