Bug 159973

Summary:

ASSERT_NOT_REACHED() in StyleBuilderConverter::convertLengthSizing() when using YouTube plugin replacement

Product:

WebKit

Reporter:

Daniel Bates <dbates>

Component:

WebCore Misc.

Assignee:

Daniel Bates <dbates>

Status:

RESOLVED DUPLICATE

Severity:

Normal

CC:

ap, bfulgham, buildbot, cdumez, commit-queue, ddkilzer, eric.carlson, esprehn+autocc, gyuyoung.kim, jer.noble, rniwa, sam, yongjun_zhang

Priority:

Version:

WebKit Local Build

Hardware:

iPhone / iPad

OS:

iOS 9.3

Attachments:

Description	Flags
Stack trace	none
Patch and Layout Test	bfulgham: review+, buildbot: commit-queue-
Archive of layout-test-results from ews102 for mac-yosemite	none
Archive of layout-test-results from ews105 for mac-yosemite-wk2	none
Archive of layout-test-results from ews115 for mac-yosemite	none

Daniel Bates

Reported 2016-07-20 09:22:07 PDT

Using a debug build of WebKit for iOS Simulator, visit a page with the following markup: [[ <!DOCTYPE html> <html> <body> <object width="425" height="350" classid="clsid:d27cdb6e-ae6d-11cf-96b8-444553540000" codebase="http://download.macromedia.com/pub/shockwave/cabs/flash/swflash.cab#version=6,0,40,0"> <embed width="425" height="350" type="application/x-shockwave-flash" src="https://www.youtube.com/v/UF8uR6Z6KLc"> </object> </body> </html> ]] Then the WebContent process will crash due to an assertion failure in StyleBuilderConverter::convertLengthSizing().

Attachments
Stack trace (3.44 KB, text/plain) 2016-07-20 09:22 PDT, Daniel Bates	no flags	Details
Patch and Layout Test (4.26 KB, patch) 2016-07-20 09:29 PDT, Daniel Bates	bfulgham: review+ buildbot: commit-queue-	Details Formatted Diff Diff
Archive of layout-test-results from ews102 for mac-yosemite (1023.48 KB, application/zip) 2016-07-20 10:03 PDT, Build Bot	no flags	Details
Archive of layout-test-results from ews105 for mac-yosemite-wk2 (926.24 KB, application/zip) 2016-07-20 10:21 PDT, Build Bot	no flags	Details
Archive of layout-test-results from ews115 for mac-yosemite (1.43 MB, application/zip) 2016-07-20 10:35 PDT, Build Bot	no flags	Details
View All Add attachment proposed patch, testcase, etc.

Daniel Bates

Comment 1 2016-07-20 09:22:53 PDT

Created attachment 284111 [details] Stack trace

Daniel Bates

Comment 2 2016-07-20 09:25:44 PDT

The assertion failure is because we assign a font weight (CSSValue100) instead of length for CSS properties width and height in YouTubeEmbedShadowElement::YouTubeEmbedShadowElement(), <https://trac.webkit.org/browser/trunk/Source/WebCore/html/shadow/YouTubeEmbedShadowElement.cpp?rev=183975#L38>. As a result, the width and height of the YouTube shadow element is effectively auto.

Daniel Bates

Comment 3 2016-07-20 09:29:12 PDT

Created attachment 284112 [details] Patch and Layout Test

Brent Fulgham

Comment 4 2016-07-20 09:55:02 PDT

Comment on attachment 284112 [details] Patch and Layout Test View in context: https://bugs.webkit.org/attachment.cgi?id=284112&action=review r=me. > Source/WebCore/ChangeLog:12 > + because a font-weight is not a CSS length. Yikes! > Source/WebCore/html/shadow/YouTubeEmbedShadowElement.cpp:-46 > - setInlineStyleProperty(CSSPropertyHeight, CSSValue100); Are you sure the right thing to do is remove these entirely? It seems like the intention was to use 100% of the Width and Height. Should we maybe be setting both of these to CSSValueWebkitFillAvailable or maybe CSSValueWebkitMaxContent? Looking at my original patch from Bug 132568, I think I probably should have done: setInlineStyleProperty(CSSPropertyWidth, 100.0, CSSPrimitiveValue::CSS_PERCENTAGE); setInlineStyleProperty(CSSPropertyHeight, 100.0, CSSPrimitiveValue::CSS_PERCENTAGE); Since the tests all pass, perhaps setting these explicitly wasn't necessary (they default to 'auto' which probably does the right thing).

Build Bot

Comment 5 2016-07-20 10:03:51 PDT

Comment on attachment 284112 [details] Patch and Layout Test Attachment 284112 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/1714504 New failing tests: plugins/youtube-plugin-replacement-assertion-fail.html

Build Bot

Comment 6 2016-07-20 10:03:56 PDT

Created attachment 284117 [details] Archive of layout-test-results from ews102 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews102 Port: mac-yosemite Platform: Mac OS X 10.10.5

Brent Fulgham

Comment 7 2016-07-20 10:14:05 PDT

(In reply to comment #5) > Comment on attachment 284112 [details] > Patch and Layout Test > > Attachment 284112 [details] did not pass mac-ews (mac): > Output: http://webkit-queues.webkit.org/results/1714504 > > New failing tests: > plugins/youtube-plugin-replacement-assertion-fail.html This looks like it's a whitespace change. I'm not sure why it fails on Mac but passes on iOS... Maybe we just need to re-baseline this one test.

Chris Dumez

Comment 8 2016-07-20 10:15:59 PDT

(In reply to comment #7) > (In reply to comment #5) > > Comment on attachment 284112 [details] > > Patch and Layout Test > > > > Attachment 284112 [details] did not pass mac-ews (mac): > > Output: http://webkit-queues.webkit.org/results/1714504 > > > > New failing tests: > > plugins/youtube-plugin-replacement-assertion-fail.html > > This looks like it's a whitespace change. I'm not sure why it fails on Mac > but passes on iOS... Maybe we just need to re-baseline this one test. It does not pass on iOS: LayoutTests/platform/ios-simulator/TestExpectations:plugins

Chris Dumez

Comment 9 2016-07-20 10:17:12 PDT

Comment on attachment 284112 [details] Patch and Layout Test View in context: https://bugs.webkit.org/attachment.cgi?id=284112&action=review > LayoutTests/plugins/youtube-plugin-replacement-assertion-fail.html:13 > + <embed width="425" height="350" type="application/x-shockwave-flash" src="https://www.youtube.com/v/UF8uR6Z6KLc"> Is it OK to have URLs to the Web in layout tests?

Build Bot

Comment 10 2016-07-20 10:21:05 PDT

Comment on attachment 284112 [details] Patch and Layout Test Attachment 284112 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/1714614 New failing tests: plugins/youtube-plugin-replacement-assertion-fail.html

Build Bot

Comment 11 2016-07-20 10:21:11 PDT

Created attachment 284120 [details] Archive of layout-test-results from ews105 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews105 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5

Alexey Proskuryakov

Comment 12 2016-07-20 10:26:03 PDT

> Is it OK to have URLs to the Web in layout tests? Simply having a URL is OK, but loading from it is not. Neither is depending on whether Flash is installed. We have a test NPAPI plugin for the tests, would it work here?

Build Bot

Comment 13 2016-07-20 10:35:24 PDT

Comment on attachment 284112 [details] Patch and Layout Test Attachment 284112 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/1714612 New failing tests: plugins/youtube-plugin-replacement-assertion-fail.html

Build Bot

Comment 14 2016-07-20 10:35:29 PDT

Created attachment 284125 [details] Archive of layout-test-results from ews115 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews115 Port: mac-yosemite Platform: Mac OS X 10.10.5

Daniel Bates

Comment 15 2016-07-20 17:15:01 PDT

(In reply to comment #4) > > Source/WebCore/ChangeLog:12 > > + because a font-weight is not a CSS length. > > Yikes! > > > Source/WebCore/html/shadow/YouTubeEmbedShadowElement.cpp:-46 > > - setInlineStyleProperty(CSSPropertyHeight, CSSValue100); > > Are you sure the right thing to do is remove these entirely? No, we want to fix the layout and rendering of this element such that it has the same height and width as its containing block (<embed>). Filed bug #159998 to address this. Removing the inline styles for height and width is sufficient to fix the assertion failure and does not cause a change in behavior (because in non-debug builds these invalid values for width and height were treated as auto - analogous to these styles not being specified). > It seems like the intention was to use 100% of the Width and Height. Should we maybe be > setting both of these to CSSValueWebkitFillAvailable or maybe > CSSValueWebkitMaxContent? Using a width of 100% causes the <embed> to expand to the width of its containing block. So, the video in the example page given by the markup in comment 0 will be the width of the viewport, which is not what we want. We want its width to be 425px. Using CSSValueWebkitFillAvailable and CSSValueWebkitMaxContent for height and width give the same sizing behavior we have today (no change).

Daniel Bates

Comment 16 2016-07-20 17:22:52 PDT

(In reply to comment #12) > > Is it OK to have URLs to the Web in layout tests? > > Simply having a URL is OK, but loading from it is not. > Neither is depending on whether Flash is installed. > Notice that the test enables plugin replacement (i.e. calls window.internals.settings.setPluginReplacementEnabled(true)). We will not attempt to load the Flash plugin; => the test does not depend on Flash being installed on the machine. > We have a test NPAPI plugin for the tests, would it work here? No, it will not work because the YouTube plugin replacement is only chosen if the type attribute of the <embed> represents Flash content.

Daniel Bates

Comment 17 2016-07-20 17:24:14 PDT

*** This bug has been marked as a duplicate of bug 159998 ***

Note You need to log in before you can comment on or make changes to this bug.