518103 - Cannot type into the phonebook search box and many keyboard shortcuts fail

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Dave Townsend [:mossop]]

On OSX and Windows 7 (untested elsewhere) we 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/20090922 Firefox/3.5 ID:20090922030618

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Just a guess, could this be a regression from some focus handling related patch?
On trunk I can't even tab to the input field.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Though, I can't reproduce this on linux.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Doesn't seem to be a regression from Bug 516076.

Regression range would be great.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Oh, I can reproduce this with *very* latest Linux build

Comment 5

[Neil Deakin]

Don't think this is focus related. May be some script or event handling bug.

A keypress event is firing and calls event.preventDefault() and event.stopPropagation(). I don't know why, but this code is being called from the 'submitOnEnter' code. The only keypress reference is from the previous block of code ('slashSearch')

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

I don't see any event handling related commits.

Comment 7

[Neil Deakin]

Probably a js issue then:

There's some code that does the following:

var SearchManager = {
  enabledBehaviorsOnInit: ["submitOnEnter"],
  initialize: function() {
    this.enabledBehaviorsOnInit.each(function(b) {
      BehaviorManager.enable(b);
    });
  },
...
}

but when the behavior.enable() is called the 'submitOnEnter' has changed to
'keypress', so may be some kind of scoping issue.

Anyway, this is getting beyond my means, so a regression window is really
needed here.

Comment 8

[Wilson Lee [:kourge]]

Here's the crucial part of the code:

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"));

BehaviorManager.register("submitOnEnter", function(e) {
  e.stop();
  window.location.hash = "#search/" + $F("text");
}.toBehavior("phonebook-search", "submit"));

These are the two main parts of the code that stop event propagation, but given the symptoms, my suspicions lean more towards slashSearch. e.stop() is a Prototype extension that calls both preventDefault() and stopPropagation() on an event.

Comment 9

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

Regressed between the following builds 09091603 - 09091703:

Pass: http://hg.mozilla.org/mozilla-central/rev/38754465ffde
Fail: http://hg.mozilla.org/mozilla-central/rev/3079370d6597

Check-ins:

http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=38754465ffde&tochange=3079370d6597

Comment 10

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

Looks like that it is one of the fixes from the Tracemonkey merge. I will run tests and bisect the range.

Comment 11

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

Yesterday I had problems in reproducing this bug with a self-made debug build. It was always working. After a restart of OS X today I can also reproduce it in the same debug build which wasn't working before.

It's definitely a fallout from the Tracemonkey merge:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=697b2063d06f&tochange=5ac5a4d5563e

Comment 12

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

It's a regression from bug 471214: "Join function objects transparently, clone via read barrier to satisfy de-facto standard". CC'ing Brendan who was working on it.

Comment 13

[Robert Sayre]

this doesn't block 1.9.2 unless we take bug 471214 there, which we won't unless this is fixed, or we mark 471214 blocking.

Comment 14

[Brendan Eich [:brendan]]

Attachment: reduced shell testcase (works on 1.9.0 shells too)

Comment 15

[Brendan Eich [:brendan]]

Attachment: fix

If (new E) ever evaluates E to a joined function object, then we had a compiler bug in selecting the wrong bytecode to evaluate E without going through the method read barrier, because E evaluated to a function and invoked via |new| has its .prototype lazily created by the operator-new opcode, and the prototype property value must not be stored on the compiler-created, shared (joined) function object. The reduced test shows this, since instanceof looks (in vain) on the unrelated clone (unjoined function object) for the right proto.

So we select GET flavored rather than CALLOP property access bytecodes where they are needed to impose the method barrier.

Everything seems happy in tests, trace-tests, etc. I had to adjust an error message to-do with E4X but the new diagnostic is an improvement.

/be

Comment 16

[Brendan Eich [:brendan]]

I'll get the reduced test into tests after conferring with bc.

/be

Comment 17

[Brendan Eich [:brendan]]

Attachment: fix, v2: tweak comment in jsemit.cpp

Comment 18

[Jason Orendorff [:jorendorff]]

Comment on attachment 404561 [details] [diff] [review]
fix, v2: tweak comment in jsemit.cpp

Maybe "callop" instead of "calling" for that boolean.

r=me with a test. AFAIK the current practice is to hg add js/src/tests/js1_8_1/regress/regress-518103.js and add a line to js/src/tests/js1_8_1/regress/jstests.list.

Comment 19

[Brendan Eich [:brendan]]

Attachment: fix to land, with test

Comment 20

[Brendan Eich [:brendan]]

(In reply to comment #18)
> (From update of attachment 404561 [details] [diff] [review])
> Maybe "callop" instead of "calling" for that boolean.

I started with that name, but it connoted JSOp so I switched to the present participle.

> r=me with a test. AFAIK the current practice is to hg add
> js/src/tests/js1_8_1/regress/regress-518103.js and add a line to
> js/src/tests/js1_8_1/regress/jstests.list.

Talked to bc, this is not anything to do with js1_8_1, so we went with ecma_3/FunExpr/regress-518103.js.

/be

Comment 21

[Brendan Eich [:brendan]]

On second thought callop is better in conjunction with the JSOP_NULL emit. Will fix before checkin.

/be

Comment 22

[Brendan Eich [:brendan]]

Attachment: final patch to commit

Comment 23

[Brendan Eich [:brendan]]

Comment on attachment 404641 [details] [diff] [review]
final patch to commit

Still welcome Igor's review, gonna land this on tm to see how the possible dup bug 520319 likes it.

/be

Comment 24

[Igor Bukanov]

Comment on attachment 404641 [details] [diff] [review]
final patch to commit

>diff --git a/js/src/jsfun.cpp b/js/src/jsfun.cpp
>+    bool isCompilerFunction() const { return !getParent(); }

This is wrong - the assert must be on the object, not the function, as it should check that the object is a function clone.

Comment 25

[Brendan Eich [:brendan]]

(In reply to comment #24)
> (From update of attachment 404641 [details] [diff] [review])
> >diff --git a/js/src/jsfun.cpp b/js/src/jsfun.cpp
> >+    bool isCompilerFunction() const { return !getParent(); }
> 
> This is wrong - the assert must be on the object, not the function, as it
> should check that the object is a function clone.

It's worse than that, I already noted on IRC this morning that top-level compile-n-go functions are not cloned and need not be, so they will botch the assertions -- but I forgot to adapt the code.

I don't see a benefit to asserting in fun_resolve about obj and not fun, though. We really don't want to resolve anything on compiler-created function objects.

/be

Comment 26

[Brendan Eich [:brendan]]

(In reply to comment #25)
> I don't see a benefit to asserting in fun_resolve about obj and not fun,
> though. We really don't want to resolve anything on compiler-created function
> objects.

Argh, of course one has to test fun == obj to discriminate between mutation of a compiler-created internal function object (which is not ok!) and mutation of a clone (which is ok).

I'll have a more thoroughly tested and self-reviewed patch up in a bit. Thanks,

/be

Comment 27

[Brendan Eich [:brendan]]

Attachment: passes jstestbrowser (jsreftests) and the usual tests/trace-tests

Comment 28

[Igor Bukanov]

Comment on attachment 404653 [details] [diff] [review]
passes jstestbrowser (jsreftests) and the usual tests/trace-tests

>+    bool internalFunctionObject(JSObject *obj) const {
>+        return obj == this && (flags & JSFUN_LAMBDA) && !getParent();
>+    }

This should be just a static function that extracts fun from obj using usual GET_FUNCTION_PRIVATE. r+ with that addressed.

Comment 29

[Igor Bukanov]

(In reply to comment #28)
> (From update of attachment 404653 [details] [diff] [review])
> >+    bool internalFunctionObject(JSObject *obj) const {
> >+        return obj == this && (flags & JSFUN_LAMBDA) && !getParent();
> >+    }
> 
> This should be just a static function that extracts fun from obj using usual
> GET_FUNCTION_PRIVATE. r+ with that addressed.

or a static inline helper outside JSFunction to avoid JSFunction:: noise.

Comment 30

[Brendan Eich [:brendan]]

Attachment: really final patch to commit with r=jorendorff

I added js_InternalFunctionObject. I've avoided "is"/"Is" verbs for other nearby predicates, and it seems worth the small savings here. Style is not consistent on this point...

/be

Comment 31

[Jason Orendorff [:jorendorff]]

Comment on attachment 404684 [details] [diff] [review]
really final patch to commit with r=jorendorff

Yep, the inline function is better.

However in the name js_InternalFunctionObject I read the implied verb as "get", not "is". You would get a similar effect if you renamed JSParseNode::isBlockChild to JSParseNode::blockChild, for example. Blah. Omitting "is" in predicates is one of those proud old-school C++ traditions I'd like to see retired.

r=me regardless.

Comment 32

[Jeff Walden [:Waldo]]

(In reply to comment #31)
> Omitting "is" in predicates is one of those proud old-school C++ traditions I'd
> like to see retired.

You were the one who wanted str->empty() over str->isEmpty(), weren't you?

In any case it seems to me a reasonable distinction can be made between adjectives like "empty" and nouns like "internalFunctionObject".  I would "is"-prefix in this case.

Comment 33

[Brendan Eich [:brendan]]

Ok you poindexters, "Is" it is!

http://hg.mozilla.org/tracemonkey/rev/e6f2cbdfce26
http://hg.mozilla.org/tracemonkey/rev/2c1129dc9062

/be

Comment 34

[Bob Clary [:bc] (inactive)]

brendan, when you add a test plz flip in-testsuite+. thx. kbye. ;-)

Comment 35

[Brendan Eich [:brendan]]

bc: will do, I sux0r lolz!

/be

Comment 37

[Jason Orendorff [:jorendorff]]

(In reply to comment #32)
> (In reply to comment #31)
> > Omitting "is" in predicates is one of those proud old-school C++ traditions I'd
> > like to see retired.
> 
> You were the one who wanted str->empty() over str->isEmpty(), weren't you?

Mmm, probably. Out of misguided adherence to what STL does, no doubt. "Prefer the Standard to the offbeat" and all that. I'm cured!

Comment 38

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/e6f2cbdfce26

Comment 39

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

Verified fixed on trunk with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.3a1pre) Gecko/20091008 Minefield/3.7a1pre ID:20091008031045

Comment 40

[Dave Townsend [:mossop]]

This is still broken for me on trunk, did this regress?

Comment 41

[Robert Sayre]

I just tried it and it worked. Give us your build details.

Comment 42

[Dave Townsend [:mossop]]

This is with a fairly clean profile in today's trunk 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 43

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

I can see this too with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.3a1pre) Gecko/20091027 Minefield/3.7a1pre ID:20091027031433

I believe we should file a new bug. Dave?

Comment 44

[Dave Townsend [:mossop]]

Filed bug 524826

Comment 45

[Bob Clary [:bc] (inactive)]

brendan, ecma_3/FunExpr/regress-518103.js made it into the 1.9.2 tree but is not listed in the manifest. If it shouldn't be run there, can you mark it skip?

Comment 46

[Bob Clary [:bc] (inactive)]

(In reply to comment #45)
> brendan, ecma_3/FunExpr/regress-518103.js made it into the 1.9.2 tree but is
> not listed in the manifest. If it shouldn't be run there, can you mark it skip?

ignore that. my mistake. sorry.