Bug 65969 – [Qt] http/tests/misc/object-embedding-svg-delayed-size-negotiation-2.htm fails intermittently

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Csaba Osztrogonác]

http/tests/misc/object-embedding-svg-delayed-size-negotiation-2.htm introduced in 88913

[Csaba Osztrogonác]

The correct result was changed to the failing result by https://trac.webkit.org/changeset/92554

[Csaba Osztrogonác]

Created attachment 103460 [details]
actual png - failing result

expected result:
http://trac.webkit.org/browser/trunk/LayoutTests/platform/qt/http/tests/misc/object-embedding-svg-delayed-size-negotiation-2-expected.png

[Csaba Osztrogonác]

you can easily reproduce it with:
Tools/Scripts/old-run-webkit-tests LayoutTests/http/tests/misc/object-embedding-svg-delayed-size-negotiation-2.htm --wait-for-httpd --iterations 100 --exit-after-n-failures --platform qt -p

test was skpped by https://trac.webkit.org/changeset/92761

[Nikolas Zimmermann]

(In reply to comment #2)
> Created an attachment (id=103460) [details]
> actual png - failing result
> 
> expected result:
> http://trac.webkit.org/browser/trunk/LayoutTests/platform/qt/http/tests/misc/object-embedding-svg-delayed-size-negotiation-2-expected.png

This is very convincing that the bug is visible on screen, not just in the DRT dumps, which I assumed so far. I wish we had all bots running pixel tests :(

Thanks, Ossy. Would you want to debug this on Qt, or do you know anyone who might want to investigate? I've a hard time reproducing this on mac - I've never been able to do it so far.

(In reply to comment #3)
> you can easily reproduce it with:
> Tools/Scripts/old-run-webkit-tests LayoutTests/http/tests/misc/object-embedding-svg-delayed-size-negotiation-2.htm --wait-for-httpd --iterations 100 --exit-after-n-failures --platform qt -p

You made my day! I had no luck in reproducing any of the svg/zoom/page/* problems on my both macs so far that were visible on gtk/qt/chromium/win (zoom-object* tests are flakey). But using this specific test, I can reproduce the problem, finally!

99 test cases (99%) succeeded
1 test case (1%) had incorrect layout

Hopefully I'm now able to find the root issue.

[Nikolas Zimmermann]

Created attachment 103753 [details]
first repro on mac slightly different png

[Nikolas Zimmermann]

Okay, I found the problem, and will upload a patch soon.
I traced this down with QtWebKit/Mac. I must say that's no fun - build-webkit --qt --debug takes 23minutes on my macbook, when editing a single .cpp file and rebuilding :(

Glad it's over now :-)

[Nikolas Zimmermann]

Created attachment 105325 [details]
Patch

Tried this patch with a from-scratch rebuilt of Qt/Mac debug, and ran the testcase 1000 tests with old-run-webkit-tests --iterations 1000 -p ...
Works w/o problems now!

[Zoltan Herczeg]

Comment on attachment 105325 [details]
Patch

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

> Source/WebCore/page/FrameView.cpp:2754
> +    HashSet<RefPtr<FrameView> > frameViews;
> +    collectFrameViewChildren(this, frameViews);

Why do you use HashSet<> here? For me it seems a simple Vector would be enough which is much faster and use less sytem resources.

[Nikolas Zimmermann]

(In reply to comment #8)
> (From update of attachment 105325 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=105325&action=review
> 
> > Source/WebCore/page/FrameView.cpp:2754
> > +    HashSet<RefPtr<FrameView> > frameViews;
> > +    collectFrameViewChildren(this, frameViews);
> 
> Why do you use HashSet<> here? For me it seems a simple Vector would be enough which is much faster and use less sytem resources.

Yes, you're just right! Uploading a revised version.

[Nikolas Zimmermann]

Created attachment 105331 [details]
Patch v2

[Zoltan Herczeg]

Comment on attachment 105331 [details]
Patch v2

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

> Source/WebCore/page/FrameView.cpp:119
> +    , m_inLayoutParentView(false)

Throw a #if ENABLE(SVG) around it.

> Source/WebCore/page/FrameView.h:403
> +    bool m_inLayoutParentView;

Ditto.

[Nikolas Zimmermann]

(In reply to comment #11)
> (From update of attachment 105331 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=105331&action=review
> 
> > Source/WebCore/page/FrameView.cpp:119
> > +    , m_inLayoutParentView(false)
> 
> Throw a #if ENABLE(SVG) around it.
> 
> > Source/WebCore/page/FrameView.h:403
> > +    bool m_inLayoutParentView;
> 
> Ditto.

Ok, shall I upload a new version?

[Nikolas Zimmermann]

Created attachment 105336 [details]
Patch v3

Uploading the final version, if we need to rollout the patch, this makes it easier.

[Zoltan Herczeg]

Comment on attachment 105336 [details]
Patch v3

Nice patch. r=me

[Nikolas Zimmermann]

Landed in r93862. Didn't cause any regressions.
Unskipped the test in r93864 on Qt, it may need a rebaseline.