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"

RESOLVED FIXED in mozilla1.9.3a1

Status

()

P1
critical
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: mossop, Assigned: brendan)

Tracking

({assertion, regression, testcase})

Trunk
mozilla1.9.3a1
assertion, regression, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking2.0 alpha1+)

Details

(Whiteboard: fixed-in-tracemonkey, URL)

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

9 years ago
+++ 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

9 years ago
blocking2.0: --- → ?
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

9 years ago
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
Blocks: 513461
Keywords: regressionwindow-wanted

Comment 10

9 years ago
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
Keywords: regressionwindow-wanted
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

9 years ago
Assignee: general → brendan

Updated

9 years ago
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.
(Assignee)

Comment 21

9 years ago
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.
(Assignee)

Comment 25

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

Comment 28

9 years ago
I think we need some resolution here soon. This makes trunk hard to use.
(Assignee)

Comment 29

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

/be
Status: NEW → ASSIGNED
(Assignee)

Comment 30

9 years ago
Got it -- what a pita to debug. Patch soon.

/be
(Assignee)

Comment 31

9 years ago
Created attachment 412772 [details] [diff] [review]
proposed fix
Attachment #412772 - Flags: review?(jorendorff)
(Assignee)

Comment 32

9 years ago
Created attachment 412775 [details] [diff] [review]
adjust expected jitstats in four trace-tests

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

9 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

9 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

9 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

9 years ago
Created attachment 413039 [details] [diff] [review]
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
Attachment #412775 - Attachment is obsolete: true
Attachment #413039 - Flags: review?(jorendorff)
Attachment #412775 - Flags: review?(jorendorff)
(Assignee)

Comment 37

9 years ago
Created attachment 413142 [details] [diff] [review]
with test
Attachment #413039 - Attachment is obsolete: true
Attachment #413142 - Flags: review?(jorendorff)
Attachment #413039 - Flags: review?(jorendorff)
(Assignee)

Updated

9 years ago
Flags: in-testsuite+
(Assignee)

Comment 38

9 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

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

9 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

9 years ago
Created attachment 413175 [details] [diff] [review]
and with trace-test
Attachment #413142 - Attachment is obsolete: true
Attachment #413175 - Flags: review+
(Assignee)

Comment 43

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

/be
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Whiteboard: fixed-in-tracemonkey
Keywords: testcase-wanted → assertion, testcase
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
(Assignee)

Updated

9 years ago
Depends on: 529837
(Assignee)

Updated

9 years ago
Blocks: 517637

Updated

9 years ago
blocking2.0: ? → alpha1

Comment 45

9 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

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