Closed Bug 1326580 Opened 7 years ago Closed 1 year ago

With unified build mode disabled, build error: PresShell.cpp:2550:8: error: code will never be executed [-Werror,-Wunreachable-code]

Categories

(Core :: Layout, defect, P3)

Product:
Core
Component:
Layout
Type:
defect

Tracking

()

Status:
RESOLVED DUPLICATE of bug 1757577
Tracking Flags:
Tracking Status
firefox53 --- affected
firefox95 --- affected

People

(Reporter: dholbert, Unassigned)

References

(Blocks 1 open bug)

Details

(Keywords: coverity, Whiteboard: [CID 1396667])

Attachments

(1 file)

The patch I pushed in comment 7
2.80 KB, patch
Details | Diff | Splinter Review 
If I disable unified builds in layout/base and fix some resulting missing-#include bustage, I'm left with this build error (with warnings-as-errors enabled):

 0:13.49 ../../../mozilla/layout/base/PresShell.cpp:2550:8: error: code will never be executed [-Werror,-Wunreachable-code]
 0:13.49   if (!aFrame->GetParent()) {
 0:13.49        ^~~~~~
 0:13.49 1 error generated.
 0:13.49 

This points to this piece of code:
======
#ifdef DEBUG
void
PresShell::VerifyHasDirtyRootAncestor(nsIFrame* aFrame)
{
  // XXXbz due to bug 372769, can't actually assert anything here...
  return;

  // XXXbz shouldn't need this part; remove it once FrameNeedsReflow
  // handles the root frame correctly.
  if (!aFrame->GetParent()) {
    return;
  }
  [...]
======

Everything after that initial "return" statement is unreachable, of course.

searchfox says this was last touched in bug 379229, back in 2007.  And the bug mentioned in the first XXX comment there (bug 372769) has been long fixed.

bz, do you know what this code should look like these days? Should we remove the initial return (the one referencing bug 372769), or just drop this function altogether, or something else?
Flags: needinfo?(bzbarsky)
(Of course, as a band-aid, I *could* easily work around this by adding an "#if 0" wrapper for everything after the initial "return;".  But it'd be preferable to actually clean up this code, if there's an obviously-correct way to do that. :))
> Should we remove the initial return

We should push that to try and see whether the tree is green.  If it is, let's remove it.  If not, then we think about what to do next.  ;)
Flags: needinfo?(bzbarsky)
Fair point!  I probably should've just done that up-front. :)

Here's an initial Try run, with the early-return removed.  Linux debug, {mochi,crash,ref,webplatform}-tests):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d83d844ba6450db03ed8c2cbc7004fb1334dd792
Looks like the try run hit lots of instances of this assertion:
###!!! ASSERTION: Frame has dirty bits set but isn't scheduled to be reflowed?: 'Not Reached', file layout/base/PresShell.cpp, line 2563

And indeed, I see that assertion spammed (for the content/child process) if I simply load (or reload) a trivial nonempty data URL, like data:text/html,a

Notes from brief GDB poking around with that testcase:
 - This function VerifyHasDirtyRootAncestor() wants to assert that this dirty frame has its ancestors all marked as having dirty children, up to the nearest (dirty) reflow root.
 - BUT, that's not actually the case, on our first layout. At that point, we have the ViewportFrame in our mDirtyRoots list, and the nsBlockFrame for the <html> node has both NS_FRAME_IS_DIRTY and NS_FRAME_HAS_DIRTY_CHILDREN set.  But its parent & grandparent & great-grandparent (the nsCanvasFrame and nsHTMLScrollFrame and ViewportFrame) only have NS_FRAME_IS_DIRTY set -- they don't have HAS_DIRTY_CHILDREN set.  So, this debugging function VerifyHasDirtyRootAncestor() complains when it walks up to the first of these & finds lack-of-knowledge about having dirty children.
 - I can work around this by making VerifyHasDirtyRootAncestor's main loop accept NS_FRAME_FIRST_REFLOW frames and walk up over them -- then, simple testcases don't spam this assertion anymore. But our chrome process still does, repeatedly, and presumably other sorts of complex content does as well.

So, more investigation is needed, to see what the modern state of this invariant should be (and to what extent we live up to that expectation).  I'm probably not diving in further at this point, though.
> - This function VerifyHasDirtyRootAncestor() wants to assert that this dirty frame has its ancestors all marked as having dirty children, up to the nearest (dirty) reflow root.

Hmm.  It's not entirely clear to me that this invariant really needs to hold for sanity.  What does need to hold is that NS_SUBTREE_DIRTY is true all the way to the reflow root.  Let me try that.
And the reason for that is that while we do in fact have dirty frames all the way up (the viewport is both NS_FRAME_IS_DIRTY and NS_FRAME_FIRST_REFLOW) we have an absolutely empty mDirtyRoots array.

Why is this happening?  Well, we're under PresShell::Initialize, which does the following, in order:

1)  Construct a bunch of frames.  These are marked with NS_FRAME_IS_DIRTY 
    and NS_FRAME_FIRST_REFLOW but probably not NS_FRAME_HAS_DIRTY_CHILDREN.
2)  Run XBL constructors.
3)  Add the root frame to mDirtyRoots.

Our asserts are firing during step 2, at least: some of the XBL constructors modify the DOM, and we try to schedule the newly-added things for reflow, and all our ancestors are NS_FRAME_IS_DIRTY and nothing is in mDirtyRoots.

Note that this shows that the assert as it stands really is wrong and should be checking NS_SUBTREE_DIRTY, not NS_FRAME_HAS_DIRTY_CHILDREN: say I insert an entire subtree at once, flush frame construction but not layout, then append a kid to a leaf of that subtree.  That leaf will be NS_FRAME_IS_DIRTY but not NS_FRAME_HAS_DIRTY_CHILDREN, so the assert would fail in that case....

Anyway, back to our Initialize/XBL friends.  I would argue the assert is catching a bug here: if the XBL constructors ask for layout info, they will get nothing useful, because mDirtyRoots is empty and hence a layout flush is a no-op.  Arguably, we should add the root frame to mDirtyRoots _before_ we run those XBL constructors.

I wonder whether we can just make that change without regressing performance.  If none of our XBL constructors perform such a flush, then I bet we can.  But if they do, we would end up doing more reflow than we do right now...

Anyway, I'll do a try push and we'll see whether fixing this problem actually makes the assert pass, then worry about whether we can actually make the change in practice.
Blocks: coverity-analysis
Keywords: coverity
Whiteboard: [CID 1396667]
Priority: -- → P3

This -Wunreachable-code warning is still reproducible when building in a debug build in non-unified mode (ac_add_options --disable-unified-build) and the "Frame has dirty bits set but isn't scheduled to be reflowed" assertion still fails in a try run.

Blocks: non-unified
status-firefox95: --- → affected
Severity: normal → S3

Can we dupe this to bug 1730741?

Flags: needinfo?(dholbert)

Looks like bug 1757577 fixed this specific issue (by adding an #if 0 wrapper); let's dupe there.

Status: NEW → RESOLVED
Closed: 1 year ago
Duplicate of bug: 1757577
Flags: needinfo?(dholbert)
Resolution: --- → DUPLICATE
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: