Closed
Bug 524826
Opened 15 years ago
Closed 15 years ago
Sometimes typing into the phonebook search box and many keyboard shortcuts fail or "Assertion failure: PCVAL_TO_OBJECT(entry->vword) == JSVAL_TO_OBJECT(v), at ../jsinterp.cpp"
Categories
(Core :: JavaScript Engine, defect, P1)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla1.9.3a1
Tracking | Status | |
---|---|---|
blocking2.0 | --- | alpha1+ |
People
(Reporter: mossop, Assigned: brendan)
References
()
Details
(Keywords: assertion, regression, testcase, Whiteboard: fixed-in-tracemonkey)
Attachments
(1 file, 4 obsolete files)
(deleted),
patch
|
brendan
:
review+
|
Details | Diff | Splinter Review |
+++ This bug was initially created as a clone of Bug #518103 +++ On OSX I cannot seem to type into the phonebook's search box. Also pressing various shortcuts for example to open the error console no longer work. This is testing with a current nightly Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.3a1pre) Gecko/20091027 Firefox/3.5 ID:20091027031433
Reporter | ||
Updated•15 years ago
|
blocking2.0: --- → ?
Comment 1•15 years ago
|
||
It not always happen. For me it only occurs sometimes. Will try to get more information tomorrow and a regression range.
Keywords: regressionwindow-wanted
Summary: Cannot type into the phonebook search box and many keyboard shortcuts fail → Sometimes typing into the phonebook search box and many keyboard shortcuts fail
Comment 2•15 years ago
|
||
I can reproduce now.
This always happens for me - mozilla-central nightlies, Windows.
Comment 4•15 years ago
|
||
While this is reproducible in official nightly builds I'm not able to repro with a self-made debug build.
Comment 5•15 years ago
|
||
(In reply to comment #4) > While this is reproducible in official nightly builds I'm not able to repro > with a self-made debug build. Disregard that comment. The minefield debug icon in the dock had the wrong location.
Comment 6•15 years ago
|
||
This regression test is totally crazy. First I tried to run a hg bisect starting from the build I have used to verify the former fix. I got bug 519194 as result which definitely cannot be the cause. Then I downloaded the builds around that date and the regression starts between 09101803 and 09101905: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=a176eb500f88&tochange=d3298de30066 I will do another bisect between those two changesets.
Comment 7•15 years ago
|
||
The interesting thing about bisecting is that even one false positive or false negative causes the result to be garbage.
Comment 8•15 years ago
|
||
Ok, now I know what happened. Testing is crazy but bisecting was probably not wrong. The problem why I got the wrong changeset is the following: When you start Minefield with a single tab open or with multiple tabs and the affected side displayed initially any keyboard entries fails. But if you have had another tab focused on shutdown and restart the phonebook tab is not displayed by default. When the HTTP auth dialog pops-up the tab is focused and keyboard entries work. Given that fact I can now search for the real regression range and will not focus the 2nd tab with the about:buildconfig page.
Comment 9•15 years ago
|
||
Changesets between the builds 09101803 and 09101905: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=a176eb500f88&tochange=d3298de30066 For me the additional fix from Daniel Holbert is causing this regression which has been only landed on trunk: http://hg.mozilla.org/mozilla-central/rev/7b0e65cf4226
Blocks: 513461
Keywords: regressionwindow-wanted
Comment 10•15 years ago
|
||
Are you sure? The only effect of that change would be to remove a compiler warning.
Comment 11•15 years ago
|
||
I run the same build a couple of times in sequence before I compiled the next one. I also went back and forward between those two builds two times. But given the problems from above I cannot tell it for sure.
Comment 12•15 years ago
|
||
(In reply to comment #9) > For me the additional fix from Daniel Holbert is causing this regression which > has been only landed on trunk: > http://hg.mozilla.org/mozilla-central/rev/7b0e65cf4226 As Neil said, that change should have zero functional effect. :) Henrik -- could you re-try those builds be absolutely sure?
Comment 13•15 years ago
|
||
I have already did that a couple of times. It would be great if someone of you could check too at least with the above mentioned nightly builds. There aren't so many patches during that day.
This is broken for me picking up the earliest m-c nightly I can see (9-28).
Comment 15•15 years ago
|
||
Ok, that was wrong again. Saving the page locally seems to be safer. That way I can second that the patch on bug 518103 hasn't changed anything and bug 513461 isn't the reason for that regression. Now I get a regression window between 09091603 and 09091703: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=38754465ffde&tochange=3079370d6597 That looks more promising. I'll bisect around the tracemonkey merge. Something I have discovered is that each keypress will hide the favicon and that it is probably related to the Behavior class which is used on that page.
No longer blocks: 513461
Keywords: regressionwindow-wanted
Comment 16•15 years ago
|
||
Sorry for the delay but I had to reinstall my MBP today. In the following it should be the correct revision which caused that regression now. Saving the web page to the local disk gives a clear result. The first bad revision is: changeset: 32658:842e6c09e35a user: Brendan Eich date: Thu Sep 03 14:41:19 2009 -0700 summary: Join lambdas assigned or initialized as methods to the compiler-created function object if we can, with a read barrier to clone on method value extractions other than call expressions (471214, r=jorendorff).
Blocks: 471214
Keywords: regressionwindow-wanted → testcase-wanted
Assignee | ||
Updated•15 years ago
|
Assignee: general → brendan
Comment 18•15 years ago
|
||
So the phonebook adds a keypress handler like so: BehaviorManager.register("slashSearch", function(e) { if (e.findElement().identify() == "text") { return; } if ((e.which || e.keyCode) == 47) { // KEY_SLASH $("text").focus(); e.stop(); } }.toBehavior(document, "keypress")); Is it possible that we're ending up inside that KEY_SLASH body for all key events for some reason?
Comment 19•15 years ago
|
||
Ah, no. The preventDefault for the keypress happens from this code: BehaviorManager.register("submitOnEnter", function(e) { e.stop(); window.location.hash = "#search/" + $F("text"); }.toBehavior("phonebook-search", "submit")); or at least so I gather from this JS stack to the preventDefault call when I type in the textfield: 0 stop(event = [object KeyboardEvent @ 0x1efaf300 (native @ 0x16d2e0f0)]) ["https://ldap.mozilla.org/phonebook/js/prototype.js":4409] this = [object Window @ 0x1eeee780 (native @ 0x1eeed790)] 1 anonymous() ["https://ldap.mozilla.org/phonebook/js/prototype.js":341] a = [object KeyboardEvent @ 0x1efaf300 (native @ 0x16d2e0f0)] this = [object KeyboardEvent @ 0x1efaf300 (native @ 0x16d2e0f0)] 2 anonymous(e = [object KeyboardEvent @ 0x1efaf300 (native @ 0x16d2e0f0)]) ["https://ldap.mozilla.org/phonebook/js/common.js":176] this = [object HTMLDocument @ 0x11cc0b30 (native @ 0x20c4ba00)] 3 anonymous(event = [object KeyboardEvent @ 0x1efaf300 (native @ 0x16d2e0f0)]) ["https://ldap.mozilla.org/phonebook/js/prototype.js":4524] this = [object HTMLDocument @ 0x11cc0b30 (native @ 0x20c4ba00)] Now why is that function hooked up as a keypress handler, exactly? That's pretty bogus.
Comment 20•15 years ago
|
||
Note that the code in comment 19 comes right after that in comment 18 in the source, in case that matters.
Assignee | ||
Comment 21•15 years ago
|
||
Yikes, I lost track of this bug. Sorry -- looking for reduction help (lithium). /be
Comment 22•15 years ago
|
||
And in fact, when this bug appears any keypress makes "#search" appear in the url bar.
Comment 23•15 years ago
|
||
Hmmm, that means the onSubmit event of <form id="phonebook-search"> is getting fired.
Comment 24•15 years ago
|
||
No, it is NOT being fired. What's being fired is the keypress event. And it's calling the wrong event handler function.
Assignee | ||
Comment 25•15 years ago
|
||
I can't reduce this easily, and it seems intermittent -- at least some of the time I get a phonebook page when running under gdb that works correctly. I'd have thought this would be deterministic. Argh. Disabling the joined function object optimization seems to cure the symptom. I'll do that tomorrow if I can't diagnose fully and fix. /be
Comment 26•15 years ago
|
||
(In reply to comment #21) > Yikes, I lost track of this bug. Sorry -- looking for reduction help (lithium). I haven't used Lithium so far and looks a bit complicated to use for the first time. Even I work on the blocklisting stuff atm. It would be great if Gary or Jesse could have a look at in minimizing the test. Or Martijn?
Comment 27•15 years ago
|
||
(In reply to comment #25) > I can't reduce this easily, and it seems intermittent -- at least some of the > time I get a phonebook page when running under gdb that works correctly. I'd > have thought this would be deterministic. Argh. I have seen the best results when I had saved this page locally. Then I was able to reproduce it all the time.
Comment 28•15 years ago
|
||
I think we need some resolution here soon. This makes trunk hard to use.
Assignee | ||
Comment 29•15 years ago
|
||
I'm close, will have this fixed one way or another today. /be
Status: NEW → ASSIGNED
Assignee | ||
Comment 30•15 years ago
|
||
Got it -- what a pita to debug. Patch soon. /be
Assignee | ||
Comment 31•15 years ago
|
||
Attachment #412772 -
Flags: review?(jorendorff)
Assignee | ||
Comment 32•15 years ago
|
||
Is this the best we can do? I will attach anyway and noodle on it... /be
Attachment #412772 -
Attachment is obsolete: true
Attachment #412775 -
Flags: review?(jorendorff)
Attachment #412772 -
Flags: review?(jorendorff)
Assignee | ||
Comment 33•15 years ago
|
||
Shapes should be static in the sense that an object literal or sequence of property sets should evolve a single shape sequence, not n distinct sequences for n executions of the script. More in a bit. /be
Assignee | ||
Comment 34•15 years ago
|
||
(In reply to comment #33) > Shapes should be static in the sense that an object literal Good so far... > or sequence of property sets ... but this assumes too much. Static analysis of assignments might discover a static shape sequence but in general it's hard: you have to know the class of the object is Object; you have to see all mutations from birth of the Object instance. > should evolve a single shape sequence, not n distinct sequences > for n executions of the script. This applies to object literals, so the jitstats fudging in the last patch is wrong and papers over a perf regression. Sorry this is dragging on -- one more patch and I'll get this done. /be
Assignee | ||
Comment 35•15 years ago
|
||
Disabling this on m-c for tonight, awaiting review on forthcoming patch (which does not regenerate shapes unnecessarily): http://hg.mozilla.org/mozilla-central/rev/d8b564f8be65 /be
Assignee | ||
Comment 36•15 years ago
|
||
Finally disentangle METHOD_BARRIER from BRANDED, by noticing that property cache filling checks both, and checks METHOD_BARRIER first; this means there is no need for METHOD_BARRIER => BRANDED, and thus we do not have to regenerate shape for each method-branded scope. Instead, the SPROP_IS_METHOD-flagged property tree node includes its method value in its identity, so it gets a unique shape shareable among all scopes referencing it. Therefore the two JSScope::methodWriteBarrier overloads must now test both BRANDED and METHOD_BARRIER. (In effect, METHOD_BARRIER is a better BRANDED flag.) Don't bother trying to reoptimize after a METHOD_BARRIER scope has one of its SPROP_IS_METHOD methods reset to another value, since we don't know how many other methods might still exist and need METHOD_BARRIER to be set (assuming BRANDED is clear). We could count but there's no point, as again it is the sprop->isMethod() condition that guards extra work off the hottest path for a METHOD_BARRIER-flagged scope. This passes the phonebook test, trace-test/*, js/src/tests, and jstestbrowser. Need a testcase still, tomorrow. /be
Attachment #412775 -
Attachment is obsolete: true
Attachment #413039 -
Flags: review?(jorendorff)
Attachment #412775 -
Flags: review?(jorendorff)
Assignee | ||
Comment 37•15 years ago
|
||
Attachment #413039 -
Attachment is obsolete: true
Attachment #413142 -
Flags: review?(jorendorff)
Attachment #413039 -
Flags: review?(jorendorff)
Assignee | ||
Updated•15 years ago
|
Flags: in-testsuite+
Assignee | ||
Comment 38•15 years ago
|
||
The test botches an assertion in debug shells: Assertion failure: PCVAL_TO_OBJECT(entry->vword) == JSVAL_TO_OBJECT(v), at ../jsinterp.cpp:2594 Bob, do I need to do more to help test drivers cope? /be
Assignee | ||
Comment 39•15 years ago
|
||
(In reply to comment #38) > The test botches an assertion in debug shells: > > Assertion failure: PCVAL_TO_OBJECT(entry->vword) == JSVAL_TO_OBJECT(v), at > ../jsinterp.cpp:2594 Unpatched debug shells, of course -- with the patch in this bug all is well. /be
Comment 40•15 years ago
|
||
Comment on attachment 413142 [details] [diff] [review] with test r=me with a test for the bug in record_JSOP_LAMBDA (which this patch also fixes).
Attachment #413142 -
Flags: review?(jorendorff) → review+
Comment 41•15 years ago
|
||
I think this test will be fine as is. It will not land on branches without the fix->all is well.
Assignee | ||
Comment 42•15 years ago
|
||
Attachment #413142 -
Attachment is obsolete: true
Attachment #413175 -
Flags: review+
Assignee | ||
Comment 43•15 years ago
|
||
http://hg.mozilla.org/tracemonkey/rev/c73182124eb7 http://hg.mozilla.org/mozilla-central/rev/b1acdba461df (with hackaround removed) /be
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Whiteboard: fixed-in-tracemonkey
Updated•15 years ago
|
Summary: Sometimes typing into the phonebook search box and many keyboard shortcuts fail → Sometimes typing into the phonebook search box and many keyboard shortcuts fail or "Assertion failure: PCVAL_TO_OBJECT(entry->vword) == JSVAL_TO_OBJECT(v), at ../jsinterp.cpp"
Comment 44•15 years ago
|
||
Tested with the latest hourly build (Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.3a1pre) Gecko/20091118 Minefield/3.7a1pre ID:20091118233844) and it works perfect. Dave is it fixed for you too?
Target Milestone: --- → mozilla1.9.3a1
Updated•15 years ago
|
blocking2.0: ? → alpha1
Comment 45•15 years ago
|
||
brendan, ecma_3/FunExpr/regress-524826.js made it into the 1.9.2 tree but isn't listed in the manifest. If it shouldn't be run can you mark it skip?
Comment 46•15 years ago
|
||
(In reply to comment #45) > brendan, ecma_3/FunExpr/regress-524826.js made it into the 1.9.2 tree but isn't > listed in the manifest. If it shouldn't be run can you mark it skip? ignore that. my mistake. sorry.
You need to log in
before you can comment on or make changes to this bug.
Description
•