Closed
Bug 518103
Opened 15 years ago
Closed 15 years ago
Cannot type into the phonebook search box and many keyboard shortcuts fail
Categories
(Core :: JavaScript Engine, defect, P1)
Core
JavaScript Engine
Tracking
()
VERIFIED
FIXED
mozilla1.9.3a1
People
(Reporter: mossop, Assigned: brendan)
References
()
Details
(Keywords: regression, Whiteboard: fixed-in-tracemonkey)
Attachments
(2 files, 5 obsolete files)
(deleted),
text/plain
|
Details | |
(deleted),
patch
|
jorendorff
:
review+
|
Details | Diff | Splinter Review |
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•15 years ago
|
||
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•15 years ago
|
||
Though, I can't reproduce this on linux.
Comment 3•15 years ago
|
||
Doesn't seem to be a regression from Bug 516076.
Regression range would be great.
Comment 4•15 years ago
|
||
Oh, I can reproduce this with *very* latest Linux build
Comment 5•15 years ago
|
||
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•15 years ago
|
||
I don't see any event handling related commits.
Comment 7•15 years ago
|
||
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.
Keywords: regressionwindow-wanted
Comment 8•15 years ago
|
||
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•15 years ago
|
||
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
Keywords: regressionwindow-wanted
Comment 10•15 years ago
|
||
Looks like that it is one of the fixes from the Tracemonkey merge. I will run tests and bisect the range.
Comment 11•15 years ago
|
||
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
Assignee: nobody → general
Component: DOM → JavaScript Engine
QA Contact: general → general
Hardware: x86 → All
Comment 12•15 years ago
|
||
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.
Blocks: 471214
Severity: normal → critical
Flags: blocking1.9.2+
Priority: -- → P1
Comment 13•15 years ago
|
||
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.
Flags: blocking1.9.2+ → blocking1.9.2?
Assignee | ||
Updated•15 years ago
|
Assignee: general → brendan
Assignee | ||
Comment 14•15 years ago
|
||
Assignee | ||
Comment 15•15 years ago
|
||
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
Attachment #404554 -
Flags: review?(jorendorff)
Assignee | ||
Comment 16•15 years ago
|
||
I'll get the reduced test into tests after conferring with bc.
/be
Flags: in-testsuite?
Assignee | ||
Comment 17•15 years ago
|
||
Attachment #404554 -
Attachment is obsolete: true
Attachment #404561 -
Flags: review?(jorendorff)
Attachment #404554 -
Flags: review?(jorendorff)
Comment 18•15 years ago
|
||
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.
Attachment #404561 -
Flags: review?(jorendorff) → review+
Assignee | ||
Comment 19•15 years ago
|
||
Attachment #404561 -
Attachment is obsolete: true
Attachment #404636 -
Flags: review?(jorendorff)
Assignee | ||
Updated•15 years ago
|
Attachment #404636 -
Flags: review?(igor)
Assignee | ||
Comment 20•15 years ago
|
||
(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
Updated•15 years ago
|
Attachment #404636 -
Flags: review?(jorendorff) → review+
Assignee | ||
Comment 21•15 years ago
|
||
On second thought callop is better in conjunction with the JSOP_NULL emit. Will fix before checkin.
/be
Assignee | ||
Comment 22•15 years ago
|
||
Attachment #404636 -
Attachment is obsolete: true
Attachment #404641 -
Flags: review+
Attachment #404636 -
Flags: review?(igor)
Assignee | ||
Comment 23•15 years ago
|
||
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
Attachment #404641 -
Flags: review?(igor)
Comment 24•15 years ago
|
||
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.
Attachment #404641 -
Flags: review?(igor) → review-
Assignee | ||
Comment 25•15 years ago
|
||
(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
Assignee | ||
Updated•15 years ago
|
Attachment #404641 -
Attachment is obsolete: true
Attachment #404641 -
Flags: review+
Assignee | ||
Comment 26•15 years ago
|
||
(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
Assignee | ||
Comment 27•15 years ago
|
||
Attachment #404653 -
Flags: review?(igor)
Assignee | ||
Updated•15 years ago
|
Attachment #404653 -
Flags: review?(jorendorff)
Updated•15 years ago
|
Attachment #404653 -
Flags: review?(igor) → review+
Comment 28•15 years ago
|
||
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•15 years ago
|
||
(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.
Assignee | ||
Comment 30•15 years ago
|
||
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
Attachment #404653 -
Attachment is obsolete: true
Attachment #404684 -
Flags: review?(jorendorff)
Attachment #404653 -
Flags: review?(jorendorff)
Updated•15 years ago
|
Attachment #404684 -
Flags: review?(jorendorff) → review+
Comment 31•15 years ago
|
||
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•15 years ago
|
||
(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.
Assignee | ||
Comment 33•15 years ago
|
||
Ok you poindexters, "Is" it is!
http://hg.mozilla.org/tracemonkey/rev/e6f2cbdfce26
http://hg.mozilla.org/tracemonkey/rev/2c1129dc9062
/be
Whiteboard: fixed-in-tracemonkey
Assignee | ||
Updated•15 years ago
|
Status: NEW → ASSIGNED
Comment 34•15 years ago
|
||
brendan, when you add a test plz flip in-testsuite+. thx. kbye. ;-)
Flags: in-testsuite? → in-testsuite+
Assignee | ||
Comment 35•15 years ago
|
||
bc: will do, I sux0r lolz!
/be
Comment 37•15 years ago
|
||
(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•15 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Comment 39•15 years ago
|
||
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
Status: RESOLVED → VERIFIED
Target Milestone: --- → mozilla1.9.3a1
Reporter | ||
Comment 40•15 years ago
|
||
This is still broken for me on trunk, did this regress?
Comment 41•15 years ago
|
||
I just tried it and it worked. Give us your build details.
Reporter | ||
Comment 42•15 years ago
|
||
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•15 years ago
|
||
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?
Reporter | ||
Comment 44•15 years ago
|
||
Filed bug 524826
Updated•15 years ago
|
Flags: blocking1.9.2? → blocking1.9.2-
Comment 45•15 years ago
|
||
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•15 years ago
|
||
(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.
You need to log in
before you can comment on or make changes to this bug.
Description
•