Hang if I edit hightlighted search result in text-input

RESOLVED FIXED

Status

()

--
critical
RESOLVED FIXED
9 years ago
6 years ago

People

(Reporter: heikki_bugzilla, Assigned: dmandelin)

Tracking

({hang, regression, testcase})

1.9.2 Branch
x86
Windows XP
hang, regression, testcase
Points:
---
Bug Flags:
blocking1.9.2 +
in-testsuite -

Firefox Tracking Flags

(status1.9.2 final-fixed)

Details

(URL)

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

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

Updated

9 years ago
Problem still on latest tracemonkey (win32).
(Assignee)

Comment 3

9 years ago
I tried this on tm tip on Mac and didn't get a hang. I'll try on Windows when I get the chance.

Updated

9 years ago
Flags: blocking1.9.2? → blocking1.9.2+

Updated

9 years ago
Assignee: general → dmandelin
(Assignee)

Comment 4

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

Comment 5

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

Comment 6

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

Updated

9 years ago
Severity: major → critical
(Assignee)

Comment 7

9 years ago
Created attachment 415235 [details] [diff] [review]
Patch

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)
(Assignee)

Updated

9 years ago
Attachment #415235 - Flags: review?(jorendorff)
(Assignee)

Comment 8

9 years ago
Comment on attachment 415235 [details] [diff] [review]
Patch

Don't review this patch--it has bugs I need to fix.
(Assignee)

Comment 9

9 years ago
Created attachment 415244 [details] [diff] [review]
Patch 2

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

Comment 12

9 years ago
http://hg.mozilla.org/mozilla-central/rev/c6e8f39cfb75
Status: NEW → RESOLVED
Last Resolved: 9 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.