Bug 141936

Summary: Setting any of the <object> element plugin controlling attributes does not have any affect
Product: WebKit Reporter: Said Abou-Hallawa <sabouhallawa>
Component: SVGAssignee: Said Abou-Hallawa <sabouhallawa>
Status: RESOLVED FIXED    
Severity: Normal CC: cmarcelo, commit-queue, dino, esprehn+autocc, gyuyoung.kim, kangil.han, koivisto, simon.fraser, webkit-bug-importer, zalan, zimmermann
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 142249    
Bug Blocks:    
Attachments:
Description Flags
test case
none
Patch
none
Patch
none
Patch
none
Patch
none
guarding test case
none
Patch none

Said Abou-Hallawa
Reported 2015-02-23 17:11:44 PST
Created attachment 247171 [details] test case Open the following test case (attached). <html> <body> <object id="object1" type="image/svg+xml" data="data:image/svg+xml, <svg xmlns='http://www.w3.org/2000/svg' width='100' height='100'> <rect width='100%' height='100%' fill='red'/> </svg>"> </object> <script> var object = document.getElementById("object1"); object.setAttribute("data", "data:image/svg+xml, \ <svg xmlns='http://www.w3.org/2000/svg' width='100' height='100'> \ <rect width='100%' height='100%' fill='lime'/> \ </svg>"); </script> </body> </html> It has an <object> tag. Its data attributes points to an SVG data uri image which loads fine. After loading the page, the data attribute to another SVG data uri image. Results: Setting the data attribute has no effect. Expected: The contents of the <object> gets updated once the data attributes changes. Notes: FireFox and Safari both work as expected.
Attachments
test case (531 bytes, text/html)
2015-02-23 17:11 PST, Said Abou-Hallawa 		no flags
Details
Patch (9.18 KB, patch)
2015-02-24 19:31 PST, Said Abou-Hallawa 		no flags
DetailsFormatted DiffDiff
Patch (8.94 KB, patch)
2015-02-24 20:19 PST, Said Abou-Hallawa 		no flags
DetailsFormatted DiffDiff
Patch (13.36 KB, patch)
2015-02-25 13:55 PST, Said Abou-Hallawa 		no flags
DetailsFormatted DiffDiff
Patch (12.99 KB, patch)
2015-02-25 14:20 PST, Said Abou-Hallawa 		no flags
DetailsFormatted DiffDiff
guarding test case (1.45 KB, text/html)
2015-03-05 11:13 PST, Said Abou-Hallawa 		no flags
Details
Patch (14.47 KB, patch)
2015-03-05 13:31 PST, Said Abou-Hallawa 		no flags
DetailsFormatted DiffDiff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1 2015-02-23 17:13:11 PST
<rdar://problem/19931466>
Said Abou-Hallawa
Comment 2 2015-02-24 19:31:47 PST
Created attachment 247291 [details] Patch
Simon Fraser (smfr)
Comment 3 2015-02-24 19:55:12 PST
Comment on attachment 247291 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=247291&action=review > LayoutTests/svg/as-object/svg-in-object-dynamic-attribute-change.html:52 > + setTimeout(function () { > + testRunner.notifyDone(); > + }, 200); 200ms is really long for a test. Can't we use an unload handler?
Said Abou-Hallawa
Comment 4 2015-02-24 20:19:19 PST
Created attachment 247300 [details] Patch
Said Abou-Hallawa
Comment 5 2015-02-24 20:20:16 PST
(In reply to comment #3) > Comment on attachment 247291 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=247291&action=review > > > LayoutTests/svg/as-object/svg-in-object-dynamic-attribute-change.html:52 > > + setTimeout(function () { > > + testRunner.notifyDone(); > > + }, 200); > > 200ms is really long for a test. Can't we use an unload handler? Done. I used the object2.onload event to notify the test runner.
zalan
Comment 6 2015-02-24 20:40:30 PST
Comment on attachment 247291 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=247291&action=review > Source/WebCore/ChangeLog:10 > + When setting any of the <object> element attributes dynamically we need > + to mark the renderer of the element to be dirty, so it can be recreated > + when needed. I don't see any renderers marked dirty in this changeset and even if there was, a dirty bit on the renderer does not mean that the renderer will be recreated. Could you also explain why the renderer needs to be recreated? (if it is recreated at all) > Source/WebCore/html/HTMLObjectElement.cpp:130 > + if (renderer() && isImageType()) { > + if (!m_imageLoader) > + m_imageLoader = std::make_unique<HTMLImageLoader>(*this); > + m_imageLoader->updateFromElementIgnoringPreviousError(); > + } else { > + invalidateRendererOnAttributeChange(name); > } > - } else if (name == classidAttr) > + } else if (name == classidAttr) { > setNeedsWidgetUpdate(true); > - else if (name == onbeforeloadAttr) > + invalidateRendererOnAttributeChange(name); > + } else if (name == onbeforeloadAttr) The changelog entry say "When setting any of the <object> element attributes...", while the changeset only deals with typeAttr, dataAttr and classidAttr. Any particular reason why those attributes are triggering style recalc while others don't? > Source/WebCore/html/HTMLObjectElement.cpp:281 > +void HTMLObjectElement::invalidateRendererOnAttributeChange(const QualifiedName& name) This functions does not invalidate a renderer as far as I can see. It simply triggers a style recalc on the element. > Source/WebCore/html/HTMLObjectElement.cpp:283 > + if (!inDocument() || isImageType() || !renderer()) Could you explain why isImageType() initiates early return? > Source/WebCore/html/HTMLObjectElement.cpp:294 > + if (invalidateRenderer) { if (!invalidaterRenderer) return; > Source/WebCore/html/HTMLObjectElement.cpp:295 > + m_useFallbackContent = false; Why?
Antti Koivisto
Comment 7 2015-02-25 02:33:58 PST
Comment on attachment 247300 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=247300&action=review > Source/WebCore/html/HTMLObjectElement.cpp:126 > + } else { > + invalidateRendererOnAttributeChange(name); > } We don't use brackets for single line code blocks. > Source/WebCore/html/HTMLObjectElement.cpp:292 > +void HTMLObjectElement::invalidateRendererOnAttributeChange(const QualifiedName& name) > +{ > + if (!inDocument() || isImageType() || !renderer()) > + return; > + > + bool invalidateRenderer = false; > + if (name == typeAttr || name == dataAttr) > + invalidateRenderer = !fastHasAttribute(classidAttr); > + else if (name == classidAttr) > + invalidateRenderer = true; > + else > + ASSERT_NOT_REACHED(); This function is called from if (name == fooAttr) tests and then it does the same tests again. Maybe there is a nicer factoring?
Antti Koivisto
Comment 8 2015-02-25 02:36:41 PST
And please address zalan's comments.
Said Abou-Hallawa
Comment 9 2015-02-25 13:55:11 PST
Created attachment 247344 [details] Patch
Said Abou-Hallawa
Comment 10 2015-02-25 14:20:02 PST
Created attachment 247346 [details] Patch
Said Abou-Hallawa
Comment 11 2015-02-25 14:34:45 PST
(In reply to comment #6) > Comment on attachment 247291 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=247291&action=review > > > Source/WebCore/ChangeLog:10 > > + When setting any of the <object> element attributes dynamically we need > > + to mark the renderer of the element to be dirty, so it can be recreated > > + when needed. > > I don't see any renderers marked dirty in this changeset and even if there > was, a dirty bit on the renderer does not mean that the renderer will be > recreated. > Could you also explain why the renderer needs to be recreated? (if it is > recreated at all) > I misunderstood the terminology. I meant we need to mark the element dirty by calling setNeedsStyleRecalc() when one of the plugin controlling attributes gets changed. I fixed the ChangeLog. > > Source/WebCore/html/HTMLObjectElement.cpp:130 > > + if (renderer() && isImageType()) { > > + if (!m_imageLoader) > > + m_imageLoader = std::make_unique<HTMLImageLoader>(*this); > > + m_imageLoader->updateFromElementIgnoringPreviousError(); > > + } else { > > + invalidateRendererOnAttributeChange(name); > > } > > - } else if (name == classidAttr) > > + } else if (name == classidAttr) { > > setNeedsWidgetUpdate(true); > > - else if (name == onbeforeloadAttr) > > + invalidateRendererOnAttributeChange(name); > > + } else if (name == onbeforeloadAttr) > > The changelog entry say "When setting any of the <object> element > attributes...", while the changeset only deals with typeAttr, dataAttr and > classidAttr. Any particular reason why those attributes are triggering style > recalc while others don't? > I meant changing the value of one of the plugin controlling attributes (classic, type and data). Changing these attributes may require recreating the the plugin. > > Source/WebCore/html/HTMLObjectElement.cpp:281 > > +void HTMLObjectElement::invalidateRendererOnAttributeChange(const QualifiedName& name) > > This functions does not invalidate a renderer as far as I can see. It simply > triggers a style recalc on the element. > Yes you are right. The element marked as need style recalc. If the old and the new styles are different, the renderer will be recreated. I fixed the ChangeLog and combined this function with HTMLObjectElement::parseAttribute() as Antti and you suggested. > > Source/WebCore/html/HTMLObjectElement.cpp:283 > > + if (!inDocument() || isImageType() || !renderer()) > > Could you explain why isImageType() initiates early return? > Your concern is valid. It turned out we need to handle the image object the same way we do for other types. The reason is if the image was rendering fall back content, the new value might fix the image object rendering. > > Source/WebCore/html/HTMLObjectElement.cpp:294 > > + if (invalidateRenderer) { > > if (!invalidaterRenderer) > return; > Done. But in HTMLObjectElement::parseAttribute(). > > Source/WebCore/html/HTMLObjectElement.cpp:295 > > + m_useFallbackContent = false; > > Why? Because if the object was rendering fall back content, the new value might fix the object rendering. The problem is we can't know right away if the new value will fix the rendering or not. If the data attribute was changing we have to wait until the resource gets loaded and parsed. And if the object can't render itself, we will fallback again.
Said Abou-Hallawa
Comment 12 2015-02-25 14:36:16 PST
(In reply to comment #7) > Comment on attachment 247300 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=247300&action=review > > > Source/WebCore/html/HTMLObjectElement.cpp:126 > > + } else { > > + invalidateRendererOnAttributeChange(name); > > } > > We don't use brackets for single line code blocks. > Done. But I changed the code a little and I combined invalidateRendererOnAttributeChange() in HTMLObjectElement::parseAttribute() as you suggested below. > > Source/WebCore/html/HTMLObjectElement.cpp:292 > > +void HTMLObjectElement::invalidateRendererOnAttributeChange(const QualifiedName& name) > > +{ > > + if (!inDocument() || isImageType() || !renderer()) > > + return; > > + > > + bool invalidateRenderer = false; > > + if (name == typeAttr || name == dataAttr) > > + invalidateRenderer = !fastHasAttribute(classidAttr); > > + else if (name == classidAttr) > > + invalidateRenderer = true; > > + else > > + ASSERT_NOT_REACHED(); > > This function is called from if (name == fooAttr) tests and then it does the > same tests again. Maybe there is a nicer factoring? Done.
WebKit Commit Bot
Comment 13 2015-02-26 10:35:10 PST
Comment on attachment 247346 [details] Patch Clearing flags on attachment: 247346 Committed r180683: <http://trac.webkit.org/changeset/180683>
WebKit Commit Bot
Comment 14 2015-02-26 10:35:14 PST
All reviewed patches have been landed. Closing bug.
WebKit Commit Bot
Comment 15 2015-03-03 16:59:27 PST
Re-opened since this is blocked by bug 142249
Said Abou-Hallawa
Comment 16 2015-03-05 11:13:26 PST
Created attachment 247972 [details] guarding test case
Said Abou-Hallawa
Comment 17 2015-03-05 11:14:07 PST
The change I did in HTMLPlugInImageElement::willRecalcStyle() the in http://trac.webkit.org/changeset/180683 was wrong not only because it makes one test flaky https://bugs.webkit.org/show_bug.cgi?id=142249 but because it causes an image object to disappear in some cases. I attached a test case which fails with run-webkit-tests always with the blamed revision. Another way to see the failure is to open the attached page and hover the image object then it will disappear.
Said Abou-Hallawa
Comment 18 2015-03-05 11:53:03 PST
The flakiness was addressed in https://bugs.webkit.org/show_bug.cgi?id=142181
Said Abou-Hallawa
Comment 19 2015-03-05 12:31:15 PST
The change I did in HTMLPlugInImageElement::willRecalcStyle() the in http://trac.webkit.org/changeset/180683 was wrong not only because it makes one test flaky https://bugs.webkit.org/show_bug.cgi?id=142181 but because it causes an image object to disappear in some cases. I attached a test case which fails with run-webkit-tests always with the blamed revision. Another way to see the failure is to open the attached page and hover the image object then it will disappear. Now I am going to explain why the test fast/css/acid2-pixel.html became flaky with this change: -- if (!useFallbackContent() && needsWidgetUpdate() && renderer() && !isImageType() && (displayState() != DisplayingSnapshot)) ++ if (!useFallbackContent() && needsWidgetUpdate() && renderer() && (displayState() != DisplayingSnapshot) In other words I will answer the question why reconstructing the image renderer of an HTMLObjectElement when HTMLPlugInImageElement::willRecalcStyle() is wrong? Depending on the Change type we pass to setNeedsStyleRecalc() we may ask the element to recreate its renderer. When the StyleRecalc timer fires or when needed, we call resolveTree() to fix the render tree. resolveTree() will call willRecalcStyle() for the dirty element. Calling setNeedsStyleRecalc(ReconstructRenderTree) or recreating the renderer might be okay for some cases but not for the RenderImage case. The RenderImage has two states. The first one is the state when the image data content is not available in which case it does not have an intrinsic size to calculate its bounding box. In this case, it returns only the padding and the border widths as the bounding box and (0, 0) as the client width. The second case when the image data content becomes available. In this case, an intrinsic size is assigned to the RenderImage so it can return the correct bounding box. Switching between these two states happens asynchronously even if the image is in the cache. A request is sent to the resource loader to get the image data content given its url. The resource loader notifies the observers (RenderImage) through CachedImage when the image is available. So if rendering, or dumping the render tree happens before the RenderImage gets notified from the resource loader regarding the availability of the resource, it will displays nothing and will have an empty client rect If we look at failure diff which happened because of the blamed revision in fast/css/acid2-pixel.html. RenderBlock (positioned) {DIV} at (48,72) size 144x24 [bgcolor=#FF0000] RenderBlock {DIV} at (0,0) size 144x0 - RenderInline {OBJECT} at (0,0) size 131x14 - RenderInline {OBJECT} at (0,0) size 131x14 - RenderImage {OBJECT} at (13,0) size 131x24 [border: none (12px solid #000000) none] + RenderInline {OBJECT} at (0,0) size 35x14 + RenderInline {OBJECT} at (0,0) size 35x14 + RenderImage {OBJECT} at (109,24) size 35x0 [border: none (12px solid #000000) none] RenderBlock (floating) {DIV} at (0,0) size 144x24 [border: none (12px solid #FF0000) none (12px solid #000000)] RenderBlock {DIV} at (0,0) size 144x24 [border: none (24px solid #FFFF00)] The RenderImage in the failed result has size = (35, 0) which is the size of the border and padding set by css. Since the image size = (96, 14), the bounding box size = (131, 14). The expected result for the RenderImage is the final bounding box while the actual result is the bounding box before attaching the image data content to the RenderImage. What happens in this case is we were asked to reconstruct the renderer of the image object and before the new RenderImage corrects its state, we capture the wrong data. Since dumping the render tree and processing events are done synchronously we should never have asked for reconstructing the renderer of an image objet. If we do so, we are going to get an intermediate state for the RenderImage and most likely we are going to get wrong results. Getting the test flaky represents the case where the dump render tree can get wrong results because it starts by resolving the style tree and immediately starts dumping the render tree. The new attached case represents the case of handling things synchronously. The mouse event for example, resolves the style tree and then the rendering the page happens immediately.
Said Abou-Hallawa
Comment 20 2015-03-05 12:48:38 PST
One last piece of information I would like to include here is the following. When fast/css/acid2-pixel.html fails, HTMLPlugInImageElement::willRecalcStyle() gets called and the following condition evaluates to true only once. if (!useFallbackContent() && needsWidgetUpdate() && renderer() && (displayState() != DisplayingSnapshot) Here is the backtrace in this case: _ZN7WebCore5StyleL11resolveTreeERNS_7ElementERNS_11RenderStyleERNS0_18RenderTreePositionENS0_6ChangeE _ZN7WebCore5StyleL11resolveTreeERNS_7ElementERNS_11RenderStyleERNS0_18RenderTreePositionENS0_6ChangeE _ZN7WebCore5StyleL11resolveTreeERNS_7ElementERNS_11RenderStyleERNS0_18RenderTreePositionENS0_6ChangeE _ZN7WebCore5StyleL11resolveTreeERNS_7ElementERNS_11RenderStyleERNS0_18RenderTreePositionENS0_6ChangeE _ZN7WebCore5StyleL11resolveTreeERNS_7ElementERNS_11RenderStyleERNS0_18RenderTreePositionENS0_6ChangeE _ZN7WebCore5StyleL11resolveTreeERNS_7ElementERNS_11RenderStyleERNS0_18RenderTreePositionENS0_6ChangeE _ZN7WebCore5StyleL11resolveTreeERNS_7ElementERNS_11RenderStyleERNS0_18RenderTreePositionENS0_6ChangeE _ZN7WebCore5StyleL11resolveTreeERNS_7ElementERNS_11RenderStyleERNS0_18RenderTreePositionENS0_6ChangeE _ZN7WebCore5Style11resolveTreeERNS_8DocumentENS0_6ChangeE _ZN7WebCore8Document11recalcStyleENS_5Style6ChangeE _ZN7WebCore8Document19updateStyleIfNeededEv _ZN7WebCore9FrameView37updateLayoutAndStyleIfNeededRecursiveEv -[WebHTMLView(WebInternal) _web_updateLayoutAndStyleIfNeededRecursive] -[WebHTMLView(WebPrivate) viewWillDraw] -[NSView viewWillDraw] -[NSView viewWillDraw] -[NSScrollView viewWillDraw] -[NSView viewWillDraw] -[NSView viewWillDraw] -[NSView viewWillDraw] -[NSView viewWillDraw] -[NSView _sendViewWillDrawInRect:clipRootView:] -[NSView displayIfNeeded] _ZL13updateDisplayv _Z4dumpv -[FrameLoadDelegate webView:locationChangeDone:forDataSource:] -[FrameLoadDelegate webView:didFinishLoadForFrame:] _Z10wtfCallIMPIP11objc_objectJP7WebViewS1_EET_PFvvES1_P13objc_selectorDpT0_ _ZL12CallDelegatePFvvEP7WebViewP11objc_objectP13objc_selectorS4_ _Z21CallFrameLoadDelegatePFvvEP7WebViewP13objc_selectorP11objc_object _ZN20WebFrameLoaderClient21dispatchDidFinishLoadEv The only place in resolveTree(), setNeedsStyleRecalc() gets called in the following: if (elementNeedingStyleRecalcAffectsNextSiblingElementStyle) { if (childElement.styleIsAffectedByPreviousSibling()) childElement.setNeedsStyleRecalc(); elementNeedingStyleRecalcAffectsNextSiblingElementStyle = childElement.affectsNextSiblingElementStyle(); } But I could not figure out whether setNeedsStyleRecalc() for the image object was called before calling resolveTree or form resolveTree itself.
Said Abou-Hallawa
Comment 21 2015-03-05 13:31:41 PST
Created attachment 247992 [details] Patch
Said Abou-Hallawa
Comment 22 2015-03-05 14:32:16 PST
Calling setNeedsStyleRecalc(ReconstructRenderTree); when one of the following attributes changes dynamically: classic, type and data, an't be avoided. If any of these attributes gets changed we need to reconstruct the renderer and to clearUseFallbackContent(). ReconstructRenderTree is a big hammer but it is never used while loading the page.
WebKit Commit Bot
Comment 23 2015-03-06 10:44:59 PST
Comment on attachment 247992 [details] Patch Clearing flags on attachment: 247992 Committed r181168: <http://trac.webkit.org/changeset/181168>
WebKit Commit Bot
Comment 24 2015-03-06 10:45:08 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.