Closed Bug 530489 Opened 15 years ago Closed 15 years ago

Hang if I edit hightlighted search result in text-input

Categories

(Core :: JavaScript Engine, defect)

Product:
Core
Component:
JavaScript Engine
Version:
1.9.2 Branch
Platform:
x86
Windows XP
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

()

Status:
RESOLVED FIXED
Tracking Flags:
Tracking Status
status1.9.2 --- final-fixed

People

(Reporter: heikki_bugzilla, Assigned: dmandelin)

References

(
URL
)

Details

(Keywords: hang, regression, testcase)

Attachments

(1 file, 1 obsolete file)

Patch 2
3.44 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review 
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2b3) Gecko/20091115 Firefox/3.6b3 (.NET CLR 3.5.30729)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2b3) Gecko/20091115 Firefox/3.6b3 (.NET CLR 3.5.30729)

Firefox jams when editing text-input which has hightlight search results

Reproducible: Always

Steps to Reproduce:
1. Make example page

<html><body><form><input type="text" value="28.02.2010"><input type="text" value="28.02.2010"><input type="text" value="28.02.2010"><input type="text" value="28.02.2010"><input type="text" value="28.02.2010"><input type="text" value="28.02.2010"></form>

2. Search for "28.02.2010" and select "Hightlight all"
3. Edit text-inputs couple of times
Actual Results:  
Firefox jams

Expected Results:  
Be able to edit
Confirmed on Windiows Vista, latest trunk. I get a Busy Script message.
For some reason I need to edit the third box first to make it hang.

The problem appears to be in:

Script: chrome://global/content/bindings/findbar.xml:521

I can't reproduce it with javascript.options.jit.chrome to false so far, but this neeeds to be confirmed.
Assignee: nobody → general
Status: UNCONFIRMED → NEW
Component: General → JavaScript Engine
Ever confirmed: true
Keywords: hang, regression, testcase
Product: Firefox → Core
QA Contact: general → general
Version: unspecified → 1.9.2 Branch
Flags: blocking1.9.2?
Summary: crash if I edit hightlighted search result in text-input → Hang if I edit hightlighted search result in text-input
Problem still on latest tracemonkey (win32).
I tried this on tm tip on Mac and didn't get a hang. I'll try on Windows when I get the chance.
Flags: blocking1.9.2? → blocking1.9.2+
Assignee: general → dmandelin
Bisection says this introduced the problem:

changeset:   31823:b7ef4e4816ed
user:        Jason Orendorff <jorendorff@mozilla.com>
date:        Tue Aug 25 15:01:29 2009 -0700
summary:     Bug 507683 part 1 - Trace native getters. r=gal.

Jason, do you have bandwidth and equipment for this (it appears to be Windows-only)?
Here is the affected code:

      <method name="_removeEditorListeners">
        <parameter name="aEditor"/>
        <body><![CDATA[
          // aEditor is an editor that we listen to, so therefore must be
          // cached. Find the index of this editor
          var idx = 0;
          while (this._editors[idx] != aEditor)
            idx++;

          // Now unhook ourselves, and remove our cached copy
          this._unhookListenersAtIndex(idx);
        ]]></body>
      </method>

Presumably this iloops. The native property get is for this._editors. If I make that abort instead of tracing, it does not lock.
I did some more investigation but the results are still confusing:

- aEditor (as inferred by reading the arg out of the spew) is not in the value returned for this._editors (printed out from GetPropertyById in jstracer.cpp).
  - So at least it makes sense that it iloops.
- |this| evaluated in the intepreter is the same value as the input object to GetPropertyId (which should be |this|).
- this._editors as computed by the tracer is equal to slot 36 in the |this| that the interpreter computes.

Given the last two points, I would think that the traced version gets exactly the same this._editors as the interpreted version. But that would make it hard to see how there could be a bug. Things I didn't check yet:

- What really is the |this._editors| as computed by the interpreter? If it's a different array that does contain aEditor, then that's the problem (i.e., the trace is not getting the right value from |this|).
- What really is the aEditor that the interpreter thinks it has? This seems much less likely to be the problem, though.
Severity: major → critical
Attached patch Patch (obsolete) — — Splinter Review 
It turns out the getter was working fine, as my previous comment suggests. It was the '!=' in the code snippet that went wrong: it was using a custom equality operation defined via JSExtendedClass, but the implementation of equality in the tracer doesn't implement those operations (it just does the standard equality test anyway). Tracing getters simply exposed the bug by making the '!=' trace where before it didn't.
Attachment #415235 - Flags: review?(jorendorff)
Attachment #415235 - Flags: review?(jorendorff)
Comment on attachment 415235 [details] [diff] [review]
Patch

Don't review this patch--it has bugs I need to fix.
Attached patch Patch 2 — — Splinter Review 
Left off a few guards (in the general sense, not the TM sense) on my first try.
Attachment #415235 - Attachment is obsolete: true
Attachment #415244 - Flags: review?(jorendorff)
Comment on attachment 415244 [details] [diff] [review]
Patch 2

Great. Thanks.
Attachment #415244 - Flags: review?(jorendorff) → review+
Comment on attachment 415244 [details] [diff] [review]
Patch 2

>     if (GetPromotedType(l) == GetPromotedType(r)) {
>         if (JSVAL_TAG(l) == JSVAL_OBJECT || JSVAL_IS_SPECIAL(l)) {
>+            if (JSVAL_TAG(l) == JSVAL_OBJECT && l) {

This breaks the (no doubt unchangeable but still) abstraction by assuming JSVAL_NULL is 0. Instead use !JSVAL_IS_PRIMITIVE(l) here, or at least l != JSVAL_NULL or !JSVAL_IS_NULL(l).

>+                JSClass *clasp = OBJ_GET_CLASS(cx, JSVAL_TO_OBJECT(l));
>+                if ((clasp->flags & JSCLASS_IS_EXTENDED) && ((JSExtendedClass*) clasp)->equality)
>+                    RETURN_STOP_A("Can't trace extended class equality operator");
>+            }
>             if (JSVAL_TAG(l) == JSVAL_OBJECT)

Since l doesn't change in the then-clause of the previous if (cited immediately above), you could common the if (JSVAL_TAG(l) == JSVAL_OBJECT), aka JSVAL_IS_OBJECT(l) test and push the next statement:

>                 op = LIR_peq;

into a new then clause conditioned by JSVAL_TAG(l) == JSVAL_OBJECT.

/be
http://hg.mozilla.org/mozilla-central/rev/c6e8f39cfb75
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Filter on qa-project-auto-change:

Bug in removed tracer code, setting in-testsuite- flag.
Flags: in-testsuite-
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: