631657 - Panorama triggering WARNING: Overflowed nscoord_MAX in conversion to nscoord

Status: RESOLVED FIXED

(Firefox Graveyard :: Panorama, defect, P3)

Description

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

I've been seeing this for a long time now but it's harmless, so I held off on reporting, but here I am now.

I caught it in GDB while resizing a group with a single tab and here's the JS stack:

0 iQClass_bounds() ["chrome://browser/content/tabview.js":393]
    rect = undefined
    this = [object Object]
1 iQClass_width() ["chrome://browser/content/tabview.js":367]
    bounds = undefined
    this = [object Object]
2 TabItems__update(tab = [object XULElement @ 0x12128c4b0 (native @ 0x120d637e0)]) ["chrome://browser/content/tabview.js":5531]
    this = [object Object]
3 TabItems_update(tab = [object XULElement @ 0x12128c4b0 (native @ 0x120d637e0)]) ["chrome://browser/content/tabview.js":5471]
    this = [object Object]
4 TabItem_setBounds(options = [object Object], immediately = true, inRect = [object Object]) ["chrome://browser/content/tabview.js":5048]
    alphaRange = undefined
    pad = undefined
    css = [object Object]
    rect = [object Object]
    validSize = [object Object]
    this = [object Object]
5 GroupItem_arrange_children_each(i = 0, child = [object Object], [object Object]) ["chrome://browser/content/tabview.js":3405]
    this = [object ChromeWindow @ 0x125d29be0 (native @ 0x125d28838)]
6 GroupItem__gridArrange(options = [object Object], box = [object Object], childrenToArrange = [object Object]) ["chrome://browser/content/tabview.js":3399]
    self = [object Object]
    index = 0
    columns = 1
    rects = [object Object]
    dropIndex = false
    result = [object Object]
    arrangeOptions = [object Object]
    this = [object Object]
7 GroupItem_arrange(options = [object Object]) ["chrome://browser/content/tabview.js":3266]
    box = [object Object]
    shouldStack = false
    childrenToArrange = [object Object]
    this = [object Object]
8 GroupItem_setBounds(options = [object Object], immediately = true, inRect = [object Object]) ["chrome://browser/content/tabview.js":2573]
    offset = [object Object]
    contentCSS = [object Object]
    titlebarCSS = [object Object]
    css = [object Object]
    titleHeight = 18
    rect = [object Object]
    validSize = [object Object]
    this = [object Object]
9 anonymous(e = [object MouseEvent @ 0x1250b95d0 (native @ 0x1250b3ae0)]) ["chrome://browser/content/tabview.js":1800]
    oldWidth = undefined
    minWidth = undefined
    box = [object Object]
    mouse = [object Object]
    this = [object ChromeWindow @ 0x105eaad40 (native @ 0x105ea93f8)]
10 anonymous(event = [object MouseEvent @ 0x1250b95d0 (native @ 0x1250b3ae0)]) ["chrome://browser/content/tabview.js":692]
    this = [object ChromeWindow @ 0x105eaad40 (native @ 0x105ea93f8)]

The undefined being used all around is suspicious to me, but could just be a red herring.

Joe: Not sure if there's anything you'd be able to say about this, but since this is hitting a warning in gfx, I figured I'd CC you.

Comment 1

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Attachment: backtrace

backtrace from gdb in case it ends up being relevant.

Comment 2

[Philipp Wagner [:imphil]]

I get the same message (and there seem to be some more similar bug reports out there), and it got quite annoying (got this every couple seconds with Panorama and playing a Flash movie).

So that's what I think happens:
- in nsBlockFrame.cpp:2833 we have
  nsRuleNode::ComputeCoordPercentCalc(aCoord, nscoord_MAX)

aCoord is:
(gdb) p aCoord
$13 = (const nsStyleCoord &) @0x7f66fb459c78: {mUnit = eStyleUnit_Percent, mValue = {mInt = 1065353216, mFloat = 1, mPointer = 0x7fffffff3f800000}}
(gdb) p aCoord.GetPercentValue()
$14 = 1

- in nsNodeRule:497 we have
  return NSToCoordFloorClamped(aPercentageBasis * aCoord.GetPercentValue());

So that's effectively calling NSToCoordFloorClamped(nscoord_MAX) and there the warning comes from.

I attached a patch that should change no functionality and get rid of the warning, but I'm not sure why the check was made with >= instead of > in the first place, so I'm not completely sure that really nothing regresses with this.

Comment 3

[Philipp Wagner [:imphil]]

Attachment: patch v1

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I suspect it might be better to make the NS_WARNING into NS_WARN_IF_FALSE instead, so you make the change for whether we warn but not whether we return.

That said, I think roc should probably review, since he wrote this (https://hg.mozilla.org/mozilla-central/rev/54946665b7c0).

Also, you should make the same change to NSToCoordFloorClamped, NSToCoordCeilClamped, and NSToCoordRoundWithClamp.

Comment 5

[Philipp Wagner [:imphil]]

Attachment: patch v2

Now using NS_WARN_IF_FALSE and I also changed all relevant functions. The new patch changes no functionality at all, only the warning messages.

Comment 6

[Philipp Wagner [:imphil]]

Can/should this go to mozilla-central before Firefox 4? Or should it wait until after the final?

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 513260 [details] [diff] [review]
patch v2

It's DEBUG-only, so a2.0=dbaron.

Comment 8

[Philipp Wagner [:imphil]]

Attachment: patch for checkin

Comment 9

[Jacek Caban]

http://hg.mozilla.org/mozilla-central/rev/cfa2a5ef17c3