524826 - 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"

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Dave Townsend [:mossop]]

+++ 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

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+1]]

It not always happen. For me it only occurs sometimes. Will try to get more information tomorrow and a regression range.

Comment 2

[Robert Sayre]

I can reproduce now.

Comment 3

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

This always happens for me - mozilla-central nightlies, Windows.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+1]]

While this is reproducible in official nightly builds I'm not able to repro with a self-made debug build.

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(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

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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

[Jason Orendorff [:jorendorff]]

The interesting thing about bisecting is that even one false positive or false negative causes the result to be garbage.

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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

Comment 10

[Neil Deakin]

Are you sure? The only effect of that change would be to remove a compiler warning.

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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

[Daniel Holbert [:dholbert]]

(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

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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.

Comment 14

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

This is broken for me picking up the earliest m-c nightly I can see (9-28).

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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.

Comment 16

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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).

Comment 18

[Boris Zbarsky [:bzbarsky]]

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

[Boris Zbarsky [:bzbarsky]]

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

[Boris Zbarsky [:bzbarsky]]

Note that the code in comment 19 comes right after that in comment 18 in the source, in case that matters.

Comment 21

[Brendan Eich [:brendan]]

Yikes, I lost track of this bug. Sorry -- looking for reduction help (lithium).

/be

Comment 22

[Boris Zbarsky [:bzbarsky]]

And in fact, when this bug appears any keypress makes "#search" appear in the url bar.

Comment 23

[Wilson Lee [:kourge]]

Hmmm, that means the onSubmit event of <form id="phonebook-search"> is getting fired.

Comment 24

[Boris Zbarsky [:bzbarsky]]

No, it is NOT being fired.  What's being fired is the keypress event.  And it's calling the wrong event handler function.

Comment 25

[Brendan Eich [:brendan]]

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

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(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

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(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

[Josh Aas]

I think we need some resolution here soon. This makes trunk hard to use.

Comment 29

[Brendan Eich [:brendan]]

I'm close, will have this fixed one way or another today.

/be

Comment 30

[Brendan Eich [:brendan]]

Got it -- what a pita to debug. Patch soon.

/be

Comment 31

[Brendan Eich [:brendan]]

Attachment: proposed fix

Comment 32

[Brendan Eich [:brendan]]

Attachment: adjust expected jitstats in four trace-tests

Is this the best we can do? I will attach anyway and noodle on it...

/be

Comment 33

[Brendan Eich [:brendan]]

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

Comment 34

[Brendan Eich [:brendan]]

(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

Comment 35

[Brendan Eich [:brendan]]

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

Comment 36

[Brendan Eich [:brendan]]

Attachment: proposed fix

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

Comment 37

[Brendan Eich [:brendan]]

Attachment: with test

Comment 38

[Brendan Eich [:brendan]]

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

Comment 39

[Brendan Eich [:brendan]]

(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

[Jason Orendorff [:jorendorff]]

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).

Comment 41

[Bob Clary [:bc] (inactive)]

I think this test will be fine as is. It will not land on branches without the fix->all is well.

Comment 42

[Brendan Eich [:brendan]]

Attachment: and with trace-test

Comment 43

[Brendan Eich [:brendan]]

http://hg.mozilla.org/tracemonkey/rev/c73182124eb7
http://hg.mozilla.org/mozilla-central/rev/b1acdba461df (with hackaround removed)

/be

Comment 44

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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?

Comment 45

[Bob Clary [:bc] (inactive)]

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

[Bob Clary [:bc] (inactive)]

(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.