Closed Bug 406769 Opened 13 years ago Closed 13 years ago

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)

Categories

(Core :: JavaScript Engine, defect, P1)

defect

Tracking

()

VERIFIED FIXED
mozilla1.9beta2

People

(Reporter: stevee, Assigned: igor)

References

Details

(Keywords: regression, testcase)

Attachments

(3 files)

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.
Flags: blocking-firefox3?
Summary: Extension All In One Gestures now throws a slow script warning dialog → Extension All In One Gestures now throws a slow script warning dialog when firefox starts up
Flags: blocking-firefox3?
Product: Firefox → Core
QA Contact: general → general
Flags: blocking1.9?
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.
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.
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.
Flags: blocking1.9? → blocking1.9-
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.
Flags: blocking1.9- → blocking1.9?
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.
Flags: blocking1.9?
Flags: blocking1.9?
Shaver?  is this a core bug or does the extension need to update...
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.
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?
I've just CC'd the author.
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 ?
Attached file 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.
Assignee: nobody → general
Component: General → JavaScript Engine
QA Contact: general → general
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).
marking blocking until we can figure out exactly what is going on.  Igor/Brendan any changes in that time range that might be relevant?
Flags: blocking1.9? → blocking1.9+
Priority: -- → P2
This can be related to bug 407501.
(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. 
Depends on: 407501
With the patch for 407501 I still got a slow warning for the test case from comment 11. So this is something else.
No longer depends on: 407501
Summary: Extension All In One Gestures now throws a slow script warning dialog when firefox starts up → 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)
Attached file Test case for js shell
The test case that currently goes into infinite loop with js shell.
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.
This is regression caused by bug 398609. The changes from bug 376957 just exposed it. As such the patch there is innocent.
Assignee: general → igor
Blocks: 398609
No longer blocks: 376957
Attached patch fix v1Splinter Review
In the patch for bug 398609 I have used the wrong ale to extract the constness status of the operation.
Attachment #292302 - Flags: review?(brendan)
It would be nice to fix this in b2 as it exposed a bad regression in the global variable optimizer.
Status: NEW → ASSIGNED
Priority: P2 → P1
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. 
I've confirmed that that patch fixes the problem for me. Thanks Igor!
Comment on attachment 292302 [details] [diff] [review]
fix v1

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

/be
Attachment #292302 - Flags: review?(brendan)
Attachment #292302 - Flags: review+
Attachment #292302 - Flags: approvalM10?
Attachment #292302 - Flags: approval1.9+
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 on attachment 292302 [details] [diff] [review]
fix v1

yep - we want this for b2..
Attachment #292302 - Flags: approvalM10? → approvalM10+
Keywords: checkin-needed
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.
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Keywords: checkin-needed
OS: Windows 2000 → All
Hardware: PC → All
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9 M10
OS: All → Windows 2000
Hardware: All → PC
OS: Windows 2000 → All
Hardware: PC → All
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
Status: RESOLVED → VERIFIED
(Gavin's testcase now works too)
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
Flags: in-testsuite+
Flags: in-litmus-
You need to log in before you can comment on or make changes to this bug.