Only clean plugin frames if !InStyleRecalc() [chromium/src : master]
Ehsan Karamad (Gerrit)
Hello Daniel. I wasn't expecting a bug on plugin so fast :). This fixes the bugs (or at least the clusterfuzz examples won't crash anymore).
To view, visit change 1073204. To unsubscribe, or for help writing mail filters, visit settings.
Ehsan Karamad (Gerrit)
Ehsan Karamad would like Daniel Cheng to review this change.
Only clean plugin frames if !InStyleRecalc()
CL 996314 introduced a new change in behavior of plugin elements and
that is unless in fallback() mode (questionably buggy condition), the
content frame is always cleaned up during DetachLayoutTree. This is fine
and desired given that plugins are design around layout and not removing
the content frame led to various bugs (as explained in more detail in CL
996314).
The new behavior has a small caveat and that is we might end up calling
DetachLayoutTree unexpectedly, i.e., during style recalc. This CL avoid
such problems by trying to detach the content frame both in the call to
DetachLayoutTree and in UpdatePlugin calls. Any remote frame will be
detached in the former call but LocalFrames might not, but will be in
the latter call which is post style recalc.
Bug: 846703, 846708
Change-Id: If651a78365f136d36702ebba56e5482154c8cdd1
---
A third_party/WebKit/LayoutTests/external/wpt/html/semantics/embedded-content/the-embed-element/embed-style-change-no-crash.html
M third_party/blink/renderer/core/html/html_plugin_element.cc
M third_party/blink/renderer/core/html/html_plugin_element.h
3 files changed, 81 insertions(+), 14 deletions(-)
Blink WPT Bot (Gerrit)
Exportable changes to web-platform-tests were detected in this CL and a pull request in the upstream repo has been made: https://github.com/w3c/web-platform-tests/pull/11172.
If this CL lands and Travis CI upstream is green, we will auto-merge the PR.
Note: Please check the Travis CI status (at the bottom of the PR) before landing this CL and only land this CL if the status is green. Otherwise a human needs to step in and resolve it manually. (This may be automated in the future, see https://crbug.com/711447)
WPT Export docs:
https://chromium.googlesource.com/chromium/src/+/master/docs/testing/web_platform_tests.md#Automatic-export-process
Daniel Cheng (Gerrit)
I think it might be nice to use DisposePluginSoon() for this. That means it'd be misnamed... but that's probably OK.
The basic idea is this:
1. We just use SetEmbeddedContentView() as we did before.
2. In html_frame_owner_element.cc, we add a helper called DisposeContentView().
3. In there, if the content view is a FrameView, we dispose it by calling Detach().
4. Otherwise, we DCHECK IsPluginView and call Dispose.
5. If in a suspend scope, we push it into the set.
6. Otherwise, call DisposeContentView(view).
The set processing will change to using DisposeContentView() instead of Dispose().
1 comment:
File third_party/blink/renderer/core/html/html_plugin_element.cc:
Patch Set #1, Line 303:
TryDetachingContentFrame();Was it also a bug to avoid calling SetEmbeddedContentView()? I see that does a lot of work.
To view, visit change 1073204. To unsubscribe, or for help writing mail filters, visit settings.
Ehsan Karamad (Gerrit)
Patch Set 1:
(1 comment)
I think it might be nice to use DisposePluginSoon() for this. That means it'd be misnamed... but that's probably OK.
The basic idea is this:
1. We just use SetEmbeddedContentView() as we did before.
2. In html_frame_owner_element.cc, we add a helper called DisposeContentView().
3. In there, if the content view is a FrameView, we dispose it by calling Detach().
4. Otherwise, we DCHECK IsPluginView and call Dispose.
5. If in a suspend scope, we push it into the set.
6. Otherwise, call DisposeContentView(view).The set processing will change to using DisposeContentView() instead of Dispose().
So I did experiment with this but so far no success. Essentially, a pathological case is:
<body> <embed src="foo-not-exists"></embed> </body>
which crashes when I load as file:///. The reason for crash is FrameLoader::CommitProvisionalLoad() on a detached frame (page_ = nullptr).
What seems to happen is that chrome-error://chromedata commits but frame is detached.
So far my understanding (which might be wrong) is that during navigation of a frame we end up calling HTMLFrameOwnerElement::SetEmbeddedContentView. Maybe this should not detach LocalFrame every time.
I'll keep investigating.
1 comment:
Patch Set #1, Line 303:
TryDetachingContentFrame();Was it also a bug to avoid calling SetEmbeddedContentView()? I see that does a lot of work.
I think so. Unfortunately went unnoticed and I believe it is fallback related again. I will see if I can repro something for it so that we can add test coverage.
To view, visit change 1073204. To unsubscribe, or for help writing mail filters, visit settings.
Rune Lillesveen (Gerrit)
FYI: I'm in the process of getting rid of the synchronous re-attach for fallback content in:
https://chromium-review.googlesource.com/c/chromium/src/+/1073421
https://chromium-review.googlesource.com/c/chromium/src/+/1072650
I don't know to what extent that will affect the changes done here, but FYI.
Blink WPT Bot (Gerrit)
Successfully updated WPT GitHub pull request with new revision "Reworking based on DisposePluginSoon code": https://github.com/web-platform-tests/wpt/pull/11172
Ehsan Karamad (Gerrit)
Ehsan Karamad uploaded patch set #4 to this change.
Delay detaching plugin frames to when plugin can be disposed
CL 996314 introduced a new change in behavior of plugin elements and
that is unless in fallback() mode (questionably buggy condition), the
content frame is always cleaned up during DetachLayoutTree. This is fine
and desired given that plugins are design around layout and not removing
the content frame led to various bugs (as explained in more detail in CL
996314).
The new behavior has a small caveat and that is we might end up calling
DetachLayoutTree unexpectedly, i.e., during style recalc. This CL avoid
such problems by trying to detach the content frame both in the call to
DetachLayoutTree and in UpdatePlugin calls. Any remote frame will be
detached in the former call but LocalFrames might not, but will be in
the latter call which is post style recalc.
Bug: 846703, 846708
Change-Id: If651a78365f136d36702ebba56e5482154c8cdd1
---
A third_party/WebKit/LayoutTests/external/wpt/html/semantics/embedded-content/the-embed-element/embed-style-change-no-crash.html
M third_party/blink/renderer/core/html/html_frame_owner_element.cc
M third_party/blink/renderer/core/html/html_frame_owner_element.h
M third_party/blink/renderer/core/html/html_plugin_element.cc
4 files changed, 81 insertions(+), 24 deletions(-)
To view, visit change 1073204. To unsubscribe, or for help writing mail filters, visit settings.
Blink WPT Bot (Gerrit)
Successfully updated WPT GitHub pull request with new revision "Edit commit message": https://github.com/web-platform-tests/wpt/pull/11172
Ehsan Karamad (Gerrit)
Patch Set 1:
FYI: I'm in the process of getting rid of the synchronous re-attach for fallback content in:
https://chromium-review.googlesource.com/c/chromium/src/+/1073421
https://chromium-review.googlesource.com/c/chromium/src/+/1072650I don't know to what extent that will affect the changes done here, but FYI.
Thanks. I guess both are landed now. I also don't see any immediate issues now but perhaps 1073421 helps with the original approach of this CL.
Blink WPT Bot (Gerrit)
Successfully updated WPT GitHub pull request with new revision "Formatting": https://github.com/web-platform-tests/wpt/pull/11172
Rune Lillesveen (Gerrit)
Patch Set 4:
Patch Set 1:
FYI: I'm in the process of getting rid of the synchronous re-attach for fallback content in:
https://chromium-review.googlesource.com/c/chromium/src/+/1073421
https://chromium-review.googlesource.com/c/chromium/src/+/1072650I don't know to what extent that will affect the changes done here, but FYI.
Thanks. I guess both are landed now. I also don't see any immediate issues now but perhaps 1073421 helps with the original approach of this CL.
Yes. It was just a heads up.
Blink WPT Bot (Gerrit)
Successfully updated WPT GitHub pull request with new revision "Do not detach frame in fallback mode": https://github.com/web-platform-tests/wpt/pull/11172
Ehsan Karamad (Gerrit)
Patch Set 1:
(1 comment)
Patch Set 1:
(1 comment)
I think it might be nice to use DisposePluginSoon() for this. That means it'd be misnamed... but that's probably OK.
The basic idea is this:
1. We just use SetEmbeddedContentView() as we did before.
2. In html_frame_owner_element.cc, we add a helper called DisposeContentView().
3. In there, if the content view is a FrameView, we dispose it by calling Detach().
4. Otherwise, we DCHECK IsPluginView and call Dispose.
5. If in a suspend scope, we push it into the set.
6. Otherwise, call DisposeContentView(view).The set processing will change to using DisposeContentView() instead of Dispose().
So I did experiment with this but so far no success. Essentially, a pathological case is:
<body> <embed src="foo-not-exists"></embed> </body>
which crashes when I load as file:///. The reason for crash is FrameLoader::CommitProvisionalLoad() on a detached frame (page_ = nullptr).What seems to happen is that chrome-error://chromedata commits but frame is detached.
So far my understanding (which might be wrong) is that during navigation of a frame we end up calling HTMLFrameOwnerElement::SetEmbeddedContentView. Maybe this should not detach LocalFrame every time.
I'll keep investigating.
Actually, I don't think we can do this inside HTMLFrameOwnerElement. Reason: FrameLoader::CommitProvisionalLoad() detaches document. So <object data="foo"></foo> will first LoadOrRedirectSubframe("foo") which creates a ContentFrame at "about:blank". Then when the actual request is committed we would end up removing the current FrameView which would essentially detach the frame unnecessarily (It actually crashes down the road).
I think the call site to remove a plugin's frame should be inside HTMLPlugInElement. PTAL.
Ehsan Karamad (Gerrit)
Ehsan Karamad uploaded patch set #8 to this change.
Delay detaching plugin frames to when plugin can be disposed
CL 996314 introduced a new change in behavior of plugin elements and
that is unless in fallback() mode (questionably buggy condition), the
content frame is always cleaned up during DetachLayoutTree. This is fine
and desired given that plugins are design around layout and not removing
the content frame led to various bugs (as explained in more detail in CL
996314).
The new behavior has a small caveat and that is we might end up calling
DetachLayoutTree unexpectedly, i.e., during style recalc. This CL avoid
such problems by trying to detach the content frame both in the call to
DetachLayoutTree and in UpdatePlugin calls. Any remote frame will be
detached in the former call but LocalFrames might not, but will be in
the latter call which is post style recalc.
Bug: 846703
Change-Id: If651a78365f136d36702ebba56e5482154c8cdd1
---
A third_party/WebKit/LayoutTests/external/wpt/html/semantics/embedded-content/the-embed-element/embed-style-change-no-crash.html
M third_party/blink/renderer/core/html/html_frame_owner_element.cc
M third_party/blink/renderer/core/html/html_frame_owner_element.h
M third_party/blink/renderer/core/html/html_plugin_element.cc
4 files changed, 79 insertions(+), 22 deletions(-)
To view, visit change 1073204. To unsubscribe, or for help writing mail filters, visit settings.
Ehsan Karamad (Gerrit)
Ehsan Karamad uploaded patch set #9 to this change.
Delay detaching plugin frames to when plugin can be disposed
CL 996314 introduced a new change in behavior of plugin elements and
that is unless in fallback() mode (questionably buggy condition), the
content frame is always cleaned up during DetachLayoutTree. This is fine
and desired given that plugins are design around layout and not removing
the content frame led to various bugs (as explained in more detail in CL
996314).
The new behavior has a small caveat and that is we might end up calling
DetachLayoutTree unexpectedly, i.e., during style recalc. This CL avoid
such problems by trying to detach the content frame both in the call to
DetachLayoutTree and in UpdatePlugin calls. Any remote frame will be
detached in the former call but LocalFrames might not, but will be in
the latter call which is post style recalc.
Bug: 846708
Change-Id: If651a78365f136d36702ebba56e5482154c8cdd1
---
A third_party/WebKit/LayoutTests/external/wpt/html/semantics/embedded-content/the-embed-element/embed-style-change-no-crash.html
M third_party/blink/renderer/core/html/html_frame_owner_element.cc
M third_party/blink/renderer/core/html/html_frame_owner_element.h
M third_party/blink/renderer/core/html/html_plugin_element.cc
4 files changed, 79 insertions(+), 22 deletions(-)
To view, visit change 1073204. To unsubscribe, or for help writing mail filters, visit settings.
Blink WPT Bot (Gerrit)
Successfully updated WPT GitHub pull request with new revision "Edit commit message": https://github.com/web-platform-tests/wpt/pull/11172
To view, visit change 1073204. To unsubscribe, or for help writing mail filters, visit settings.
Daniel Cheng (Gerrit)
2 comments:
File third_party/blink/renderer/core/html/html_frame_owner_element.h:
Patch Set #9, Line 136:
void DisposePluginSoon(EmbeddedContentView*);This should be DisposeContentViewSoon
File third_party/blink/renderer/core/html/html_plugin_element.cc:
if (ContentFrame() && !UseFallbackContent()) {
// In case the element is going to show fallback content, leave the
// content frame attached. Note: this is almost certainly buggy.
// Make sure the frame is detached when it is safe to dispose of plugin
// content.
DisposePluginSoon(ContentFrame()->View());
}I think we should just fix the error path at this point, as it's quite complicated to follow:
Can we just change https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/loader/document_loader.cc?rcl=024402fa85d6d160c5dc90d71f6bd54dabe55b86&l=417 to check if we're using fallback content and not continue with the error page load if we're using fallback content? It doesn't really make sense in that case.
Then we ought to be able to use SetEmbeddedContentView(nullptr) uniformly (with SetEmbeddedContentView updated to remove the special case for plugins and to just use DisposeContentViewSoon for everything)
To view, visit change 1073204. To unsubscribe, or for help writing mail filters, visit settings.
Blink WPT Bot (Gerrit)
Successfully updated WPT GitHub pull request with new revision "Rebased": https://github.com/web-platform-tests/wpt/pull/11172
Blink WPT Bot (Gerrit)
Successfully updated WPT GitHub pull request with new revision "Detach Frame at UpdatePlugin": https://github.com/web-platform-tests/wpt/pull/11172
Blink WPT Bot (Gerrit)
Successfully updated WPT GitHub pull request with new revision "Detach Frame When Needed At UpdatePlugin": https://github.com/web-platform-tests/wpt/pull/11172
To view, visit change 1073204. To unsubscribe, or for help writing mail filters, visit settings.
Ehsan Karamad (Gerrit)
Not necessarily proposing this as a fix, but this would stop the issues we currently have, probably not worse than what we had before 996314.
This seems to also fix 848283 (that is not the goal of CL though).
2 comments:
File third_party/blink/renderer/core/html/html_plugin_element.cc:
Patch Set #12, Line 221:
if (ContentFrame() && !detach_content_frame_at_update_)Maybe 'NeedsPluginUpdate()' is not needed.
File third_party/blink/renderer/core/loader/document_loader.cc:
Patch Set #12, Line 424:
return;I don't know if this is OK (not changing any state here).
To view, visit change 1073204. To unsubscribe, or for help writing mail filters, visit settings.
Ehsan Karamad (Gerrit)
Ehsan Karamad abandoned this change.
To view, visit change 1073204. To unsubscribe, or for help writing mail filters, visit settings.