Bug 144528 – Assertion failure (!needsLayout()) loading inkedmag.com

• Component: New Bugs
• Status: RESOLVED FIXED

[Simon Fraser (smfr)]

Assertion failure (!needsLayout()) loading inkedmag.com

[Simon Fraser (smfr)]

Created attachment 252241 [details]
Patch

[Simon Fraser (smfr)]

Created attachment 252242 [details]
Patch

[Simon Fraser (smfr)]

rdar://problem/20788681

[Darin Adler]

Comment on attachment 252242 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=252242&action=review

GTK and EFL build errors are both:
Source/WebCore/rendering/RenderReplaced.cpp:143:5: error: 'SetLayoutNeededForbiddenScope' is not a member of 'WebCore::RenderObject'

r=me assuming you fix the release build, but please consider my comments too

> Source/WebCore/platform/graphics/BitmapImage.cpp:91
> +void BitmapImage::startTimer(double delay)

In new code, we should be using standard library types, not doubles, for time and duration values. I think std::chrono::milliseconds might be what we want here and it’s probably worth considering rewriting the code to use it.

> Source/WebCore/platform/graphics/BitmapImage.cpp:94
> +    m_frameTimer = std::make_unique<Timer>(*this, &BitmapImage::advanceAnimation);
> +    m_frameTimer->startOneShot(delay);

Should we assert there is no existing timer? If there is, then destroying it is a side effect that doesn’t seem like part of the function name “start timer”.

Surprisingly low level graphics class to have a timer in it. I realize you were just refactoring, but I am still surprised. In modern code we would use a lambda instead of a pointer to member function for the timer.

> Source/WebCore/platform/graphics/BitmapImage.h:339
> +    bool m_pendingNotifyAfterCaughtUpToLastFrame : 1;

I find this name confusing. The noun here seems to be “notify” as in “this is a notify”, which doesn’t make sense. How would you describe this boolean flag in colloquial English? Maybe that would point to a better name for it.

> Source/WebCore/rendering/RenderReplaced.cpp:143
> +    RenderObject::SetLayoutNeededForbiddenScope scope(this);

Looks like this class is defined for builds without NDEBUG only. Would be better if it was a harmless inlined no-op in those builds, but otherwise you need to put #ifndef NDEBUG around this. Also, it should take a RenderObject&, not a RenderObject*.

Shouldn’t need the RenderObject:: prefix here. This is a member function of a class derived from RenderObject so that should already be in scope without an explicit qualifier.

[Simon Fraser (smfr)]

(In reply to comment #4)
> Comment on attachment 252242 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=252242&action=review
> 
> GTK and EFL build errors are both:
> Source/WebCore/rendering/RenderReplaced.cpp:143:5: error:
> 'SetLayoutNeededForbiddenScope' is not a member of 'WebCore::RenderObject'
> 
> r=me assuming you fix the release build, but please consider my comments too
> 
> > Source/WebCore/platform/graphics/BitmapImage.cpp:91
> > +void BitmapImage::startTimer(double delay)
> 
> In new code, we should be using standard library types, not doubles, for
> time and duration values. I think std::chrono::milliseconds might be what we
> want here and it’s probably worth considering rewriting the code to use it.

I'd prefer to switch the entire class over to std::chrono later.

> > Source/WebCore/platform/graphics/BitmapImage.cpp:94
> > +    m_frameTimer = std::make_unique<Timer>(*this, &BitmapImage::advanceAnimation);
> > +    m_frameTimer->startOneShot(delay);
> 
> Should we assert there is no existing timer? If there is, then destroying it
> is a side effect that doesn’t seem like part of the function name “start
> timer”.

Added assert.

> Surprisingly low level graphics class to have a timer in it. I realize you
> were just refactoring, but I am still surprised. In modern code we would use
> a lambda instead of a pointer to member function for the timer.

Agreed; timer cleanup can come in a separate patch.

> > Source/WebCore/platform/graphics/BitmapImage.h:339
> > +    bool m_pendingNotifyAfterCaughtUpToLastFrame : 1;
> 
> I find this name confusing. The noun here seems to be “notify” as in “this
> is a notify”, which doesn’t make sense. How would you describe this boolean
> flag in colloquial English? Maybe that would point to a better name for it.

Renamed.

> > Source/WebCore/rendering/RenderReplaced.cpp:143
> > +    RenderObject::SetLayoutNeededForbiddenScope scope(this);
> 
> Looks like this class is defined for builds without NDEBUG only. Would be
> better if it was a harmless inlined no-op in those builds, but otherwise you
> need to put #ifndef NDEBUG around this. Also, it should take a
> RenderObject&, not a RenderObject*.

Wrapping in #ifndef NDEBUG for now, will clean up in a separate patch.

> Shouldn’t need the RenderObject:: prefix here. This is a member function of
> a class derived from RenderObject so that should already be in scope without
> an explicit qualifier.

Fixed.

[Simon Fraser (smfr)]

Created attachment 252245 [details]
For EWS

[Simon Fraser (smfr)]

Comment on attachment 252245 [details]
For EWS

Bring Darin's review forward.

[Simon Fraser (smfr)]

http://trac.webkit.org/changeset/183732

[Alexey Proskuryakov]

This broke a test on Windows: https://webkit-test-results.appspot.com/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=fast%2Fimages%2Fanimated-gif-body-outside-viewport.html

Should we roll out?

[Simon Fraser (smfr)]

No, I'll take a look when I get in soon.

[Simon Fraser (smfr)]

Windows test skipped in https://trac.webkit.org/r183756.