Closed Bug 524826 Opened 11 years ago Closed 11 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)

defect

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)

+++ 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
blocking2.0: --- → ?
It not always happen. For me it only occurs sometimes. Will try to get more information tomorrow and a regression range.
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
I can reproduce now.
This always happens for me - mozilla-central nightlies, Windows.
While this is reproducible in official nightly builds I'm not able to repro with a self-made debug build.
(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.
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.
The interesting thing about bisecting is that even one false positive or false negative causes the result to be garbage.
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.
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
Are you sure? The only effect of that change would be to remove a compiler warning.
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.
(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?
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).
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
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).
Assignee: general → brendan
Duplicate of this bug: 527403
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?
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.
Note that the code in comment 19 comes right after that in comment 18 in the source, in case that matters.
Yikes, I lost track of this bug. Sorry -- looking for reduction help (lithium).

/be
And in fact, when this bug appears any keypress makes "#search" appear in the url bar.
Hmmm, that means the onSubmit event of <form id="phonebook-search"> is getting fired.
No, it is NOT being fired.  What's being fired is the keypress event.  And it's calling the wrong event handler function.
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
(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?
(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.
I think we need some resolution here soon. This makes trunk hard to use.
I'm close, will have this fixed one way or another today.

/be
Status: NEW → ASSIGNED
Got it -- what a pita to debug. Patch soon.

/be
Attached patch proposed fix (obsolete) — Splinter Review
Attachment #412772 - Flags: review?(jorendorff)
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)
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
(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
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
Attached patch proposed fix (obsolete) — Splinter Review
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)
Attached patch with test (obsolete) — Splinter Review
Attachment #413039 - Attachment is obsolete: true
Attachment #413142 - Flags: review?(jorendorff)
Attachment #413039 - Flags: review?(jorendorff)
Flags: in-testsuite+
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
(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 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+
I think this test will be fine as is. It will not land on branches without the fix->all is well.
Attachment #413142 - Attachment is obsolete: true
Attachment #413175 - Flags: review+
http://hg.mozilla.org/tracemonkey/rev/c73182124eb7
http://hg.mozilla.org/mozilla-central/rev/b1acdba461df (with hackaround removed)

/be
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: fixed-in-tracemonkey
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"
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
Depends on: 529837
Blocks: 517637
blocking2.0: ? → alpha1
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?
(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.