1447405 - Valgrind cries at nsFrame::ComputeSize in stylo-only builds.

Status: RESOLVED FIXED

(Core :: Layout, defect, P3)

Description

[Emilio Cobos Álvarez (:emilio)]

It's likely a false positive from the unconditional branches that get removed (it's likely to be optimized differently).

I'm going to land an expectation change pending further investigation sin Julian told me that he was working on updating Valgrind in the tree to catch more cases like these.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

https://treeherder.mozilla.org/logviewer.html#?job_id=169237353&repo=mozilla-inbound is the log.

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Attachment: Patch for the suppression.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e289c669eea2
Add a valgrind suppression on a CLOSED TREE. rpending=jseward

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8960723 [details] [diff] [review]
Patch for the suppression.

Per IRC discussion this was indeed a false-positive, but the better fix is to initialize the relevant boolean in nsFrame::ComputeSize. Julian said he was looking into it.

Comment 5

[Julian Seward [:jseward]]

I believe this is happening because, in nsFrame::ComputeSize(), at the
following two places

5731
  if (maxISizeCoord.GetUnit() != eStyleUnit_None &&
      !(isFlexItem && usingFlexBasisForISize)) {

5742
  if (minISizeCoord.GetUnit() != eStyleUnit_Auto &&
      !(isFlexItem && usingFlexBasisForISize)) {

the subterm

  (isFlexItem && usingFlexBasisForISize)

is compiled as

  (usingFlexBasisForISize && isFlexItem)

This is correct (ignoring the performance effects) because the compiler can
see that isFlexItem is false when usingFlexBasisForISize is undefined.  The
code preceding the conditionals has the structure

  bool isFlexItem = parentFrame && parentFrame->IsFlexContainerFrame() &&
    !parentFrame->HasAnyStateBits(NS_STATE_FLEX_IS_EMULATING_LEGACY_BOX) &&
    !HasAnyStateBits(NS_FRAME_OUT_OF_FLOW);
  ..
  bool usingFlexBasisForISize;
  if (isFlexItem) {
    ..
    usingFlexBasisForISize =
      (flexContainerIsRowOriented == inlineAxisSameAsParent);    
    ..
  }

So after this point, either
  isFlexItem is true and usingFlexBasisForISize is defined
or
  isFlexItem is false and usingFlexBasisForISize is undefined.

Unfortunately the transformation causes a false warning from Memcheck,
because it assumes that every conditional branch in a program is
"important".

A simple "fix" is to initialise usingFlexBasisForISize to false so as to
keep Valgrind happy.  We've added similar initialisations in Gecko a few
times in the past.

Comment 6

[Julian Seward [:jseward]]

Attachment: bug1447405-nsFrame-inits-1.diff

A simple fix.

Comment 7

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8960909 [details] [diff] [review]
bug1447405-nsFrame-inits-1.diff

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

r=me, with the valgrind suppression removed from the tree too.

Thanks for looking into this!

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ed12db196ee9
Valgrind cries at nsFrame::ComputeSize in stylo-only builds.  r=emilio.

Comment 10

[Noemi Erli[:noemi_erli]]

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

Comment 11

[Jet Villegas (inactive)]

[Triage 2018/03/23 - P3]

Comment 12

[Julian Seward [:jseward]]

Can this be closed now?

Comment 13

[Emilio Cobos Álvarez (:emilio)]

Yeah, probably.