1205604 - Height value ignored (treated as 'auto') once it reaches 17895698px

Status: NEW

(Core :: CSS Parsing and Computation, defect)

Description

[Benjamin Bouvier [:bbouvier] (inactive)]

STR:
- go to https://www.unrealengine.com/html5
- open developers console
- wait for the "Successfully compiled asm.js code" message
- click on the link to the source code next to this message (UE4Game-HTML5 etc)
- click on "prettify source"
- wait a bit (it's a bit slow -- don't know if there's something to do here, or if it's worth another bug)
- try to select all code (Ctrl+A or right click and "Select All")

Expected:
- all prettified code is selected

Observed:
- only the first line is selected

Comment 1

[Jan Keromnes [:janx]]

Well, that particular file is very big (I count 3551631 lines when prettified).

I was able to reproduce your issue, but I had a hard time even opening the file at all, let alone prettifying the code (firefox-bin at 150% CPU, lots of browser freezes, and even a "codemirror.js unresponsive" prompt). I'm not sure what you were trying to achieve by selecting all the code, given that copy/pasting somewhere between 10MB and 2GB of text is generally a bad idea today.

Your "select all" bug is valid, given that only the first line shows up as highlighted, but this might be somewhat of an edge case. Still, I think we should fix the highlight issue (which might be on codemirror's side), and I believe we could still drastically improve the performance of our editor, especially when dealing with large files.

Comment 2

[Jan Keromnes [:janx]]

I believe it's a visual problem, because I tried copy/pasting after a Ctrl+A and got bitten severely (especially while pasting).

For what it's worth, when I try it in Chromium with the devtools open (their editor is also based on codemirror), the tab crashes while downloading the ASM.js code, and says something about closing tabs to free up memory even though my RAM stays under 60% usage.

Comment 3

[Jan Keromnes [:janx]]

However, still in Chromium, when I leave the devtools closed, I'm able to start the game.

When I open the devtools afterwards, I'm able to open the file (after much CPU stress), see it syntax highlighted (Firefox didn't bother), but when I try to prettify it, the toolbox dies after a short while.

Both browsers are able to "select all" when unprettified.

Comment 4

[Jan Keromnes [:janx]]

Brian or James, is this something you'd like to have a look at?

Comment 5

[Brian Grinstead [:bgrins]]

Attachment: long-prettified-js.txt.zip

I see the same problem on the CodeMirror home page when pasting in the contents of the prettified file into the demo CodeMirror instance there.  Basic STR:

Unzip this and copy the contents of the file onto the clipboard
Paste it into the CM area on codemirror.net (may have to dismiss some slow script warnings if it takes a long time to complete)
Press ctrl+a

Expected:
All of the contents are highlighted

Actual:
Only the first line is highlighted

Note that pressing ctrl+c at this point *does* copy all of the contents onto the clipboard

Marijn, can you reproduce this?  Would it be best to file an issue on the CodeMirror repository for this?

Comment 6

[James Long (:jlongster)]

Something that huge is going to be limited on the amount of stuff we can do with it. Usually when people are debugging asm.js apps they use sourcemaps, which splits it into several normal size files. I'm not going to be able to look at this this week, but I'm skeptical if there's anything we can do (that's going to be a CodeMirror issue, not ours).

Comment 7

[Marijn Haverbeke]

Hi Brian. I couldn't paste in the whole file (Firefox 40, Linux, seems to refuse to paste beyond a certain data size), but I could reproduce the issue when I pasted only the first million lines. The problem appears to be that setting CSS properties to huge numbers (in this case, the editor computes its content height as 18 million pixels) seems to cause Firefox to ignore (or strangely interpret) those properties. For the same reason, the scroll bar and gutter are broken when such a large file is present.

Chrome handles this much better -- both the pasting of big blobs and the large CSS numbers -- so I think you should look at the browser itself here.

Comment 8

[Brian Grinstead [:bgrins]]

Hi Cam, based on Comment 7 it sounds like there may be an issue with calculating or rendering the height of the faked blue selection element in CodeMirror when it gets to around 18 million pixels tall.  Here is a simple test case showing the same problem: http://jsfiddle.net/f7ykwqax/1/.  I'm not sure exactly who to direct this to - can you help?

Comment 9

[Cameron McCormack (:heycam)]

Gecko stores coordinate values as app units, using 32 bit integers.  That means for a 1dppx display, 18 million pixels would be stored as 18,000,000 * 60 = 0x405F7E00.  nscoord_MAX is 0x40000000 https://dxr.mozilla.org/mozilla-central/source/gfx/src/nsCoord.h#48 so we've got an element height that we're not guaranteed we process correctly.

It's possible we should be using the "saturating" arithmetic functions on nscoord values somewhere where we aren't.  Mats, is this something you know about?

Comment 10

[Mats Palmgren (inactive)]

(In reply to Cameron McCormack (:heycam) from comment #9)
> It's possible we should be using the "saturating" arithmetic functions on
> nscoord values somewhere where we aren't.

That sounds plausible.

Comment 11

[Brian Grinstead [:bgrins]]

Found the magic number, it looks like 17895698px.  1px less and the full height is displayed.  See this test page: 

data:text/html,<style>.toobig {height: 17895698px; background: lightblue;} .normal { height: 17895697px; background: limegreen; </style><div class="toobig">17895698px million pixels tall</div><div class="normal">17895697px million pixels tall</div>

Comment 12

[Boris Zbarsky [:bzbarsky]]

> Found the magic number, it looks like 17895698px.

Right, floor(0x40000000 / 60) + 1.  See comment 9.

Comment 13

[Jukka Jylänki]

Here is another STR:

1. Download https://dl.dropboxusercontent.com/u/40949268/emcc/bugs/2015-08-28-emunittest_0.4-AngryBots-u5.1.3f1_hg-e1.34.6-release-devbuild-exceptions.zip and unzip.
2. Open index.html and debug the sources.
3. The vertical scrollbar is grayed out on file Development/2015-08-28-emunittest_0.4-AngryBots-u5.1.3f1_hg-e1.34.6-release-devbuild-exceptions.js

Comment 14

[James Long (:jlongster)]

How hard would this be to fix? Any chance someone could work on it?

Comment 15

[Brian Grinstead [:bgrins]]

(In reply to James Long (:jlongster) from comment #14)
> How hard would this be to fix? Any chance someone could work on it?

Setting ni? flags

Comment 16

[Brian Grinstead [:bgrins]]

Attachment: large-height-scroll-regression-test.patch

I added a regression test for this in layout/generic/test/test_bug1205604.html.  In the process I realized there's also an off-by-one error with an element of 17895697px size where clientHeight returns 17895696.  This can be seen in the test case in Comment 11.

Comment 17

[Cameron McCormack (:heycam)]

I'm not really sure what the principles are behind choosing to use the saturating nscoord arithmetic methods or not.  I suspect that it would be a fair amount of work to ensure all the layout codepaths that lead to the overflowing height value here don't ever overflow nscoord_MAX.

I don't recall now which bit of code is interpreting this overflowed value in a way that causes only the first line's selection to be painted.  Maybe there's a quick fix that clamps something to nscoord_MAX late in the process, but I couldn't be sure without investigating.

Comment 18

[Mats Palmgren (inactive)]

I've nothing to add to what Cameron said.

Comment 19

[Daniel Holbert [:dholbert]]

(In reply to Cameron McCormack (:heycam) (busy January 30 – February 6) from comment #9)
> It's possible we should be using the "saturating" arithmetic functions on
> nscoord values somewhere where we aren't.

I don't think adding more "saturating" arithmetic would help here, unless we either:
 - changed the ceiling where we clamp saturating arithmetic (currently clamped to nscoord_MAX)
...or:
 - changed the sentinel value for NS_AUTOHEIGHT (currently also nscoord_MAX)

In the testcase in comment 11, it looks like "height: 17895698px" ends up producing a height of nscoord_MAX app-units, which happens to be the same as our NS_AUTOHEIGHT sentinel value.  So, that's why we lay out the box as if it had 'height:auto'.

Comment 20

[Cameron McCormack (:heycam)]

(In reply to Daniel Holbert [:dholbert] from comment #19)
> So, that's why we lay out the box as if it had 'height:auto'.

Aha!  Would it make sense to change NS_AUTOHEIGHT to be a value that is less likely to be produced, i.e. so that when we max out our coords (assuming we're clamping correctly), that max value doesn't coincide with NS_AUTOHEIGHT?  https://dxr.mozilla.org/mozilla-central/source/layout/generic/nsIFrame.h?from=NS_AUTOHEIGHT#149 suggests it may or may not be easy to do.

Comment 21

[Daniel Holbert [:dholbert]]

That's my second bullet-point from comment 19.  That could help a bit here, but I'm not sure it's worth it; layout would still end up broken (possibly horrendously so) around wherever we clamp.

So things would likely still be broken; they'd just be differently-broken.