property added to DOM element somehow got missing after scrolling

RESOLVED FIXED in Firefox 22

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: allenlee8, Assigned: bz)

Tracking

({regression})

23 Branch
mozilla24
x86
Mac OS X
regression
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox21 wontfix, firefox22+ verified, firefox23+ fixed, firefox24+ fixed)

Details

Attachments

(3 attachments)

(Reporter)

Description

5 years ago
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_8_3) AppleWebKit/536.29.13 (KHTML, like Gecko) Version/6.0.4 Safari/536.29.13

Steps to reproduce:

Refer to https://github.com/SitePen/dgrid/issues/597

A property columnId is added programatically to the DOM element created by createElement.  After some scrolling, it no longer exists.


Actual results:

The extended DOM element property got lost after some scrolling.


Expected results:

The added property should remain there.

Comment 1

5 years ago
Could you provide a simple testcase (html file embedding JS libs if necessary), please.
Component: General → DOM
Flags: needinfo?(allenlee8)
Keywords: testcase-wanted
(Reporter)

Comment 2

5 years ago
Created attachment 754662 [details]
Test case

To test it, please open test.html
Flags: needinfo?(allenlee8)
(Reporter)

Comment 3

5 years ago
The problem occurs in Grid.js under the dgrid folder.  Basically the row cells are created in the createRowCells function (line 78).  The dom element of the row cell is added the property columnId in line 113.  When the grid attempts to move down one row, it will call the cell function (line 30) to match the elements against the columnId in line 62.   At that point, the columnId property somehow cannot be found, causing the grid failure to find the element hence stop scrolling.   I found that the problem is resolved if line 113 is changed from cell.columnId = id to Object.defineProperty(cell, "columnId", {value: id}). 

Although there is a workaround, this could potentially be a bigger problem.

Updated

5 years ago
Attachment #754662 - Attachment mime type: application/octet-stream → application/zip

Comment 4

5 years ago
I'm not able to reproduce the issue with FF21/FF24 on Win 7. Is it specific to Mac OSX?
Flags: needinfo?(allenlee8)
I can't reproduce either with the attached testcase in a current Mac nightly, but I'm not sure whether I'm following the right steps to reproduce.  What are the exact steps with the attached testcase?

Also, which exact builds are you seeing the problem with?  You should be able to get the build ids from about:buildconfig: it's the http://hg.mozilla.org url there.

I agree that this sounds really bad and we need to sort it out...
(Reporter)

Comment 6

5 years ago
I tried the latest build  http://hg.mozilla.org/mozilla-central/rev/c190422547ed. The problem persists.  I'm using Macbook Air (2GHz i7) with OSX 10.8.3.
Flags: needinfo?(allenlee8)
(Reporter)

Comment 7

5 years ago
By the way, FF 21/22 does not have such a problem.  It starts in FF 23.
OK, then presumably I'm not doing the right things to reproduce this.  Can you please list explicit step by step instructions to reproduce using the attached testcase?
Flags: needinfo?(allenlee8)
Alternately, are you willing to use http://harthur.github.com/mozregression/ to narrow down when the problem appeared?  But a way to reproduce the problem will likely still be needed to fix it...
(Reporter)

Comment 10

5 years ago
One more thing, I found that if I test this while the debugger in FF is on, this problem does not occur.  To reproduce the problem, make sure you do not have debugger/console on.  After the problem occurs, you can then turn on the debugger to set breakpoint at line 62 of Grid.js.  You can then see columnId property missing.
Flags: needinfo?(allenlee8)
(Reporter)

Comment 11

5 years ago
I'll try to see if I can make a video on that first.  mozregression seems to be a good idea.  I'll try that too.
Per comment 10, sounds like a JIT issue of some sort...  Perhaps failing to call class defineProperty hooks?

> I'll try to see if I can make a video on that first.

No need for a video; just a list of steps to reproduce, as if you were telling someone over a phone call how to do it.  Start with "load the page" and then say exactly where to click and which keys to press.
Assignee: nobody → general
Component: DOM → JavaScript Engine
Flags: needinfo?(allenlee8)
(Reporter)

Comment 13

5 years ago
Steps to reproduce the problem:
1. decompress the attached test case zip file into a folder.
2. open FF and load test.html in the folder.
3. You should then see a number of grids. In the first grid, click the cell of the first data row under Column 1 (i.e. the cell with the word "normal"). You should see a cell highlighted with the dotted line border.
4. Hold down the Down Array Key and you will see the dotted line selection move down row by row, and scroll up the rows when it reaches the lower bound of grid.
5. After a while, the scrolling stops at a particular row.  (note: when you use the mouse to scroll up the grid, you can see that there are more rows below this row.)
6. At this point, you can open the debugger and set breakpoint at line 62 of Grid.js. The problem occurs in the following lines:

var elements = rowElement.getElementsByTagName("td");
for(var i = 0; i < elements.length; i++){
  if(elements[i].columnId == columnId){   <<--- no columnId property in elements[i]
Flags: needinfo?(allenlee8)
(Reporter)

Comment 14

5 years ago
For step 6, after you set the breakpoint, you should click back to the cell where the scroll stops at, and then press Down Arrow Key.
Thank you for the steps to reproduce!

The regression range on nightlies seems to be http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=acf388eaf9e9&tochange=fef5f202b2dc

Furthermore, turning off Ion compilation makes the bug go away.
Status: UNCONFIRMED → NEW
Ever confirmed: true
And of course in a debug build from rev fef5f202b2dc the problem doesn't reproduce....
hg bisect claims, if I did not screw it up:

The first bad revision is:
changeset:   129587:9aff2a52d88b
user:        Brian Hackett <bhackett1024@gmail.com>
date:        Mon Apr 22 20:22:30 2013 -0600
summary:     Bug 863518 - Consider types added by loop body when unboxing OSR values, r=dvander.
Blocks: 863518
tracking-firefox23: --- → ?
tracking-firefox24: --- → ?
Keywords: testcase-wanted → regression
Blocks: 861807
Created attachment 755441 [details]
Standalone testcase

After some discussion with bz on IRC I was able to write this testcase. It creates a large table, adds a property to every cell, forces a GC and then checks if the properties still exist.

It prints "FAIL", with Ion disabled it prints "PASS".
Since this may be related to 861807, I'll take a look.
Assignee: general → kvijayan
(Reporter)

Comment 20

5 years ago
I have some interesting observations.  Not sure if it helps.  In the testcase attached in Comment 18, if the statement cell.y = "1" is added below cell.x = "foo", the problem goes away.
From yesterday's discussion on IRC...

IsPropertyAddInlineable in IonCaches.cpp is the most likely culprit.
Created attachment 756028 [details] [diff] [review]
Make sure to not skip calling addProperty hooks when objects have them.  Otherwise DOM expandos can go AWOL.

This seems to do the trick.  The test fails without the patch and passes with it.
Attachment #756028 - Flags: review?(kvijayan)
Assignee: kvijayan → bzbarsky
Whiteboard: [need review]
Comment on attachment 756028 [details] [diff] [review]
Make sure to not skip calling addProperty hooks when objects have them.  Otherwise DOM expandos can go AWOL.

Review of attachment 756028 [details] [diff] [review]:
-----------------------------------------------------------------

Jan's test case also passes with this.  I'll check if bug 861807 is also resolved by this.
Attachment #756028 - Flags: review?(kvijayan) → review+
http://hg.mozilla.org/integration/mozilla-inbound/rev/db53707fb4d7
status-firefox23: --- → affected
status-firefox24: --- → fixed
Whiteboard: [need review]
Target Milestone: --- → mozilla24
So I was wondering why exactly this popped up now, but it's obvious: BC lowered the iteration count needed for an ion-compile, so actual pages with a few thousand elements started having DOM manipulation code like this get ion-compiled.

If I modify Jan's testcase to have 10000 rows instead of 100 (so 2e5 iterations instead of 2e3), then it fails in beta and Fx21 release and Fx20 release.  Seems to pass in Fx19, but we might still have had a resolve hook on nodes at that point or something.

I think we should land this on both Aurora and Beta.
Comment on attachment 756028 [details] [diff] [review]
Make sure to not skip calling addProperty hooks when objects have them.  Otherwise DOM expandos can go AWOL.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Combination of WebIDL bindings and Ion
   compiler, exacerbated by the baseline compiler on Aurora 23.
User impact if declined: Some websites are randomly broken.  This is a more
   likely outcome in 23 than 22.
Testing completed (on m-c, etc.): Passes attached testcase.
Risk to taking this patch (and alternatives if risky): I believe this is
   low-risk: it simply disables an optimization when it's not valid to make it.
String or IDL/UUID changes made by this patch: None
Attachment #756028 - Flags: approval-mozilla-beta?
Attachment #756028 - Flags: approval-mozilla-aurora?
status-firefox21: --- → wontfix
status-firefox22: --- → affected
tracking-firefox22: --- → ?

Updated

5 years ago
Duplicate of this bug: 861807
https://hg.mozilla.org/mozilla-central/rev/db53707fb4d7
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
(Reporter)

Comment 29

5 years ago
Can this fix be applied to firefox 23?
That's what the approval requests are about.  Once (well, "if", in theory) the branch release drivers approve it, it'll get landed on the branches.
Comment on attachment 756028 [details] [diff] [review]
Make sure to not skip calling addProperty hooks when objects have them.  Otherwise DOM expandos can go AWOL.

The wontfix for FF21 implies that this isn't a FF22 regression, and comment 7 suggests that there are no known user impacts in FF22. Leaning towards minusing for FF22, but of course approving for Aurora 23 in the meantime.
Attachment #756028 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Updated

5 years ago
tracking-firefox23: ? → +
tracking-firefox24: ? → +
Alex, it's pretty easy to create an HTML page that triggers this bug in FF22.  See comment 25.

There might not be any we've heard about yet, because they'd need to be pretty big pages, or at least cycle through lots of elements.  But there are certainly big-enough pages out there to trigger it; they might just not have yet.

I think the risk here is low enough we should just take it on 22 to avoid nasty surprises when Facebook publishes a new feature or something.  ;)
https://hg.mozilla.org/releases/mozilla-aurora/rev/4acfd400902a
status-firefox23: affected → fixed

Updated

5 years ago
tracking-firefox22: ? → +
Comment on attachment 756028 [details] [diff] [review]
Make sure to not skip calling addProperty hooks when objects have them.  Otherwise DOM expandos can go AWOL.

bz's spidey sense hasn't failed us yet :). low risk, so we'll take the fix.
Attachment #756028 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
https://hg.mozilla.org/releases/mozilla-beta/rev/752b1880d266
status-firefox22: affected → fixed
This issue is verified fixed with Firefox 22 beta 4 (build ID: 20130605070403) on a Mac OSX 10.8.3 machine in 32bit mode, if I test the standalone testcase from comment 18.

On the other hand, while checking with the testcase attached in comment 2 on the same machine, I can't see any difference between the Nightly from 2013-05-24 and Firefox 22 beta 4. While following the STR from comment 13, after step 4, the scrolling still stops at the last but one row, instead of the last row. And after setting the breakpoint at line 62 of Grid.js, in the right I still see the columnId property having "0-0" value.

Does anyone have any suggestions? Thanks!

Updated

5 years ago
QA Contact: manuela.muntean

Updated

5 years ago
Flags: needinfo?
As far as I can see, that's just the behavior of this library because of that column with row-spanning cells.  allenlee8@netvigator.com, can you confirm?
Flags: needinfo? → needinfo?(allenlee8)
(Reporter)

Comment 38

5 years ago
Yes, it is intended to behave like this.
Flags: needinfo?(allenlee8)
In this case, I'm marking this verified on Firefox 22, based on comment 37 and comment 38. Thanks!
status-firefox22: fixed → verified

Comment 40

5 years ago
I'm curious what a JavaScript shell testcase would look like.
Well, for a start you need a shell object with an addProperty hook...
You need to log in before you can comment on or make changes to this bug.