876098 - property added to DOM element somehow got missing after scrolling

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[allenlee8]

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

[Loic]

Could you provide a simple testcase (html file embedding JS libs if necessary), please.

Comment 2

[allenlee8]

Attachment: Test case

To test it, please open test.html

Comment 3

[allenlee8]

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.

Comment 4

[Loic]

I'm not able to reproduce the issue with FF21/FF24 on Win 7. Is it specific to Mac OSX?

Comment 5

[Boris Zbarsky [:bzbarsky]]

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...

Comment 6

[allenlee8]

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.

Comment 7

[allenlee8]

By the way, FF 21/22 does not have such a problem.  It starts in FF 23.

Comment 8

[Boris Zbarsky [:bzbarsky]]

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?

Comment 9

[Boris Zbarsky [:bzbarsky]]

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...

Comment 10

[allenlee8]

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.

Comment 11

[allenlee8]

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.

Comment 12

[Boris Zbarsky [:bzbarsky]]

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.

Comment 13

[allenlee8]

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]

Comment 14

[allenlee8]

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.

Comment 15

[Boris Zbarsky [:bzbarsky]]

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.

Comment 16

[Boris Zbarsky [:bzbarsky]]

And of course in a debug build from rev fef5f202b2dc the problem doesn't reproduce....

Comment 17

[Boris Zbarsky [:bzbarsky]]

hg bisect claims, if I did not screw it up:

The first bad revision is:
changeset:   129587:9aff2a52d88b
user:        Brian Hackett <[e-mail]>
date:        Mon Apr 22 20:22:30 2013 -0600
summary:     Bug 863518 - Consider types added by loop body when unboxing OSR values, r=dvander.

Comment 18

[Jan de Mooij [:jandem]]

Attachment: 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".

Comment 19

[Kannan Vijayan [:djvj]]

Since this may be related to 861807, I'll take a look.

Comment 20

[allenlee8]

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.

Comment 21

[Boris Zbarsky [:bzbarsky]]

From yesterday's discussion on IRC...

IsPropertyAddInlineable in IonCaches.cpp is the most likely culprit.

Comment 22

[Boris Zbarsky [:bzbarsky]]

Attachment: 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.

Comment 23

[Kannan Vijayan [:djvj]]

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.

Comment 24

[Boris Zbarsky [:bzbarsky]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/db53707fb4d7

Comment 25

[Boris Zbarsky [:bzbarsky]]

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 26

[Boris Zbarsky [:bzbarsky]]

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

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/db53707fb4d7

Comment 29

[allenlee8]

Can this fix be applied to firefox 23?

Comment 30

[Boris Zbarsky [:bzbarsky]]

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 31

[Alex Keybl [:akeybl]]

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.

Comment 32

[Boris Zbarsky [:bzbarsky]]

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

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/4acfd400902a

Comment 34

[Alex Keybl [:akeybl]]

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.

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/752b1880d266

Comment 36

[Manuela Muntean [Away]]

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!

Comment 37

[Boris Zbarsky [:bzbarsky]]

As far as I can see, that's just the behavior of this library because of that column with row-spanning cells.  [e-mail], can you confirm?

Comment 38

[allenlee8]

Yes, it is intended to behave like this.

Comment 39

[Manuela Muntean [Away]]

In this case, I'm marking this verified on Firefox 22, based on comment 37 and comment 38. Thanks!

Comment 40

[Jesse Ruderman]

I'm curious what a JavaScript shell testcase would look like.

Comment 41

[Boris Zbarsky [:bzbarsky]]

Well, for a start you need a shell object with an addProperty hook...