Bug 82566

Created attachment 134486 [details] Patch

Attachment 134486 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/ChangeLog:11: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5] Total errors found: 1 in 2 files If any of these errors are false positives, please file a bug against check-webkit-style.

I'm not really certain if it is the right thing to do, since it changes the layout behavior, and Element:scrollWidth seems to do some magic calculation with zoom factors. Also I'm not sure what to do with RenderTextControlSingleLine::setScrollLeft/Top.

Comment on attachment 134486 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=134486&action=review > I'm not really certain if it is the right thing to do, since it changes the layout behavior, The tests are passing on cr-linux EWS so how come it changes the layout? What kind of changes do you see? It sounds like you should add a test that shows this difference? A test would also help us see the issue and advise a better way to fix it (I think your approach is sane but would love to see the incriminating test case). > Source/WebCore/ChangeLog:3 > + RenderTextControlSingleLine::scrollWidth/H/L/T should not call back to DOM tree Please don't abbreviate, it makes your change less readable. > Source/WebCore/rendering/RenderTextControlSingleLine.cpp:694 > + if (innerTextElement() && innerTextElement()->renderBox()) I wonder if we need any of the NULL-checks. Does it make sense to have a RenderTextControlSingleLine without an inner element? (the code is not consistent about those unfortunately) I would have to look at this logic but tkent@ may know from the top of his head. > Source/WebCore/rendering/RenderTextControlSingleLine.cpp:695 > + return innerTextElement()->renderBox()->scrollWidth(); As you pointed out, there is a difference with respect to zoom factor between Element::scrollWidth and RenderBox::scrollWidth. To be truly accurate here, we should take the innerTextElement()'s zoom factor into account in the same way Element::scrollWidth() does. I do fear code duplication here but I don't see a good way around that as we don't want to expose the inner gut of Element::scrollWidth to everyone.

Created attachment 136327 [details] Patch

(In reply to comment #4) > (From update of attachment 134486 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=134486&action=review > > > I'm not really certain if it is the right thing to do, since it changes the layout behavior, > > The tests are passing on cr-linux EWS so how come it changes the layout? What kind of changes do you see? It sounds like you should add a test that shows this difference? A test would also help us see the issue and advise a better way to fix it (I think your approach is sane but would love to see the incriminating test case). I didn't phrase it right. What I meant to say is that it "might" change layout behavior because we no longer request the latest layout from Element. As the bots are all green I guess I worried too much. > > Source/WebCore/rendering/RenderTextControlSingleLine.cpp:694 > > + if (innerTextElement() && innerTextElement()->renderBox()) > > I wonder if we need any of the NULL-checks. Does it make sense to have a RenderTextControlSingleLine without an inner element? (the code is not consistent about those unfortunately) > > I would have to look at this logic but tkent@ may know from the top of his head. > > Source/WebCore/rendering/RenderTextControlSingleLine.cpp:695 > > + return innerTextElement()->renderBox()->scrollWidth(); > > As you pointed out, there is a difference with respect to zoom factor between Element::scrollWidth and RenderBox::scrollWidth. To be truly accurate here, we should take the innerTextElement()'s zoom factor into account in the same way Element::scrollWidth() does. I do fear code duplication here but I don't see a good way around that as we don't want to expose the inner gut of Element::scrollWidth to everyone. For real I have no idea what is going on(apply no idea dog meme here). Anyway the newly uploaded patch mimic the original behavior as much as possible, and we will go from there?

Comment on attachment 136327 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=136327&action=review > Source/WebCore/ChangeLog:12 > + No new tests. No change in behavior. As asked once but I did not get any answer: do you have a test case for that change? I don't fear the change in behavior (very unlikely), more the badness of having layout called recursively. > Source/WebCore/rendering/RenderTextControlSingleLine.cpp:695 > + RenderBox* rend = innerTextElement()->renderBox(); Again, don't abbreviate. > Source/WebCore/rendering/RenderTextControlSingleLine.cpp:714 > + return adjustForAbsoluteZoom(rend->scrollLeft(), rend); If you are going to update the getters, I would like the setters to be updated in the same way.

(In reply to comment #7) > (From update of attachment 136327 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=136327&action=review > > > Source/WebCore/ChangeLog:12 > > + No new tests. No change in behavior. > > As asked once but I did not get any answer: do you have a test case for that change? > I don't fear the change in behavior (very unlikely), more the badness of having layout called recursively. No, I mean the opposite. There should be no visible difference in layout behavior, and that is what we want. I can't observe anything weird at this time. > > Source/WebCore/rendering/RenderTextControlSingleLine.cpp:695 > > + RenderBox* rend = innerTextElement()->renderBox(); > > Again, don't abbreviate. Ugh that's the same copy & paste from Element. Anyway I'll change it to "box", sounds better than "rend". > > Source/WebCore/rendering/RenderTextControlSingleLine.cpp:714 > > + return adjustForAbsoluteZoom(rend->scrollLeft(), rend); > > If you are going to update the getters, I would like the setters to be updated in the same way. Alright. Seems like the setter on Element just calls the setter on the RenderBox anyway.

Created attachment 137437 [details] Patch

trchen, I don't really understand what problem this patch is solving. Is this just a stylistic improvement, or is there a concrete benefit from this patch?

(In reply to comment #10) > trchen, I don't really understand what problem this patch is solving. Is this just a stylistic improvement, or is there a concrete benefit from this patch? Working on the render tree should not trigger layout behavior, otherwise the tree can mutate while we're still traversing on it. This fixes a crash we have in downstream.

Can we include a test case for the crash?

It's fine if the test case doesn't crash yet (as long as it will crash when we finish upstreaming).

(In reply to comment #12) > Can we include a test case for the crash? I can try but I don't know exactly know how to trigger this. Basically we want to find a situation that calling layout once won't stabilize the render tree, leaving a single-line textbox that is dirty, and doing a layout on that textbox will cause its ancestor to be removed from the tree. The original crash is from a complex webpage has lots of javascript and animation (and the crash is hardly reproducible). Not easy to isolate a minimal test case.

As a general principle, we require tests for patches that fix crashes so that we don't regress the crash in the future. Of course, sometimes we're not able to produce test cases, but that's the exception rather than the rule. I don't know enough about this part of the code to know what's best here. Hopefully jchaffraix or eric can guide us to the right solution.

I think the invariant we need to be testing for here is that we should not be triggering a layout by calling these getters. We don't need to explicitly test for a crash, as that's just one of the things that can go wrong due to that underlying bug. I don't know how to check the timing of when a layout happens in a layout test, however.

(In reply to comment #16) > I think the invariant we need to be testing for here is that we should not be triggering a layout by calling these getters. We don't need to explicitly test for a crash, as that's just one of the things that can go wrong due to that underlying bug. I can think of 2 ways to test: * mutate the shadow DOM, mutate the <input> tag and then force a layout on the <input> (not sure it would crash but it's possible as we could recursively call layout in this case) * expose a method to get the internal scroll dimensions using Internals, mutate the shadow DOM and compare the values before / after the mutation. > I don't know how to check the timing of when a layout happens in a layout test, however. The only direct way I can think of is through the web-inspector. The other methods would be indirect (timing difference in the operation in JavaScript as layout adds some overhead, ...).

Another way to test is to add ASSERT()s in C++ code that forbid this codepath, then make a layout test that hits it and have the test be "no ASSERT()s should fire in debug mode". I think that would make more sense in this case.

Comment on attachment 137437 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=137437&action=review > Source/WebCore/ChangeLog:9 > + Calling to Element::scrollWidth/scrolHeight/scrollLeft/scrollTop/ > + setScrollLeft/setScrollTop will call > + Document::updateLayoutIgnorePendingStyleSheets, which can trigger > + unexpected layout update. How is that possible? When we're calling scrollWidth on RenderTextControlSingleLine via Element::scrollWidth(), we should have already called updateLayoutIgnorePendingStylesheets(). The call to updateLayoutIgnorePendingStyleSheets() here should be no-op. Who is the caller? We need to fix that caller to call updateLayoutIgnorePendingStyleSheets first. r- because this is a wrong fix. Do we have a crash report / stack trace for this? > Source/WebCore/ChangeLog:11 > + Reviewed by NOBODY (OOPS!). This line should appear before the long description. > Source/WebCore/ChangeLog:13 > + No new tests. No change in behavior. There definitely is a behavior change. Please revise.

(In reply to comment #19) > (From update of attachment 137437 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=137437&action=review > > > Source/WebCore/ChangeLog:9 > > + Calling to Element::scrollWidth/scrolHeight/scrollLeft/scrollTop/ > > + setScrollLeft/setScrollTop will call > > + Document::updateLayoutIgnorePendingStyleSheets, which can trigger > > + unexpected layout update. > > How is that possible? When we're calling scrollWidth on RenderTextControlSingleLine via Element::scrollWidth(), > we should have already called updateLayoutIgnorePendingStylesheets(). > The call to updateLayoutIgnorePendingStyleSheets() here should be no-op. Who is the caller? > We need to fix that caller to call updateLayoutIgnorePendingStyleSheets first. r- because this is a wrong fix. scrollWidth() is a virtual function. Anything that calls renderBox->scrollWidth() can potentially go to RenderTextControlSingleLine::scrollWidth(). Why is it a wrong fix when there is no layout test fails? Why query innerTextElement()->scrollWidth() from the DOM tree when there is the cached value in the render tree? > Do we have a crash report / stack trace for this? Unfortunately this crash currently only exists in downstream Chrome for Android builds. http://b.corp.google.com/issue?id=6101477 http://crash.corp.google.com/reportdetail?reportid=098fe31738dfc304 http://b.corp.google.com/issue?id=6237967 http://crash.corp.google.com/reportdetail?reportid=7facd3bd586667d4 The two crashes are very similar in nature. We have a RenderLayer::updateNonFastScrollableRegion function in downstream that collects all scrollable region in a RenderLayer, so the compositor thread will know when to delegate scrolling events to the WebKit thread. The function traverse through the render tree and query RenderBox::canBeScrolledAndHasScrollableArea() which will call scrollWidth(). Then unexpected layout screwed up the tree traversal. We delayed the call until very last moment of a compositor commit, even after we called WebViewImpl::layout() for a full layout update, but layout can still happen unexpectedly. As far as I know layout is an iterative operation. The render tree can take multiple layouts to reach to a fixed point. Means that it is virtually impossible to safely traverse the render tree while querying geometries from the RenderObjects, if you allow a RenderObject to re-layout anytime as it wishes. > > Source/WebCore/ChangeLog:11 > > + Reviewed by NOBODY (OOPS!). > > This line should appear before the long description. > > > Source/WebCore/ChangeLog:13 > > + No new tests. No change in behavior. > > There definitely is a behavior change. Please revise. Okay, we'll find a way to test this, but before that let's determine what is the correct fix.

@trchen: It's fine to add tests that only crash in the chromium-android branch as long as the exercise the code you're changing here. Progress on this patch seems to have stalled out, and I'm unsure how we should proceed. It sounds like trchen is still looking for feedback on the approach. @rniwa: What do you think of the approach, understanding that this patch isn't complete yet due to missing tests?

Comment on attachment 137437 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=137437&action=review >>> Source/WebCore/ChangeLog:9 >>> + unexpected layout update. >> >> How is that possible? When we're calling scrollWidth on RenderTextControlSingleLine via Element::scrollWidth(), >> we should have already called updateLayoutIgnorePendingStylesheets(). >> The call to updateLayoutIgnorePendingStyleSheets() here should be no-op. Who is the caller? >> We need to fix that caller to call updateLayoutIgnorePendingStyleSheets first. r- because this is a wrong fix. >> >> Do we have a crash report / stack trace for this? > > scrollWidth() is a virtual function. Anything that calls renderBox->scrollWidth() can potentially go to RenderTextControlSingleLine::scrollWidth(). > > Why is it a wrong fix when there is no layout test fails? Why query innerTextElement()->scrollWidth() from the DOM tree when there is the cached value in the render tree? I stand on my ground and agree with Tien-Ren logic here: the overflow logic, that runs during layout, will query scrollWidth. This can end up in us triggering a new layout (which is slow but normally harmless). I don't deny that I don't know the condition for that to happen, just that it is an undeniable possibility. If we put the overflow logic aside, it's a layering violation that rendering/ calls back to DOM in this case. Fixing callers is unfeasible for several reasons: RenderTextControlSingleLine have a very different way of working than the average RenderObject (ie it wraps a shadow DOM) but mostly because you *don't* want rendering to start calling Document::updateLayoutIgnorePendingStyleSheets >>> Source/WebCore/ChangeLog:11 >>> + Reviewed by NOBODY (OOPS!). >> >> This line should appear before the long description. > > Okay, we'll find a way to test this, but before that let's determine what is the correct fix. There is no mention of where the "Reviewed by ..." line should be in the coding style or any tool enforcing what you said, Ryosuke. I don't think a contributor should be penalized for that nor that a reviewer should impose his view on that.

This patch does not appear to be needed anymore.

(In reply to comment #23) > This patch does not appear to be needed anymore. I don't think so. The crash bug was very difficult to reproduce in the first place. This patch is stalled only because there are other stuffs that are in more urgent .:p

Ok, please feel free to re-open this bug or create a new bug when you've got a way to reproduce the crash.