Skip to content

GitLab

T
tangled
Commit e85090e8 authored by Dominic Farolino's avatar Dominic Farolino Committed by Commit Bot
Browse files
Options

Revert "Link element events should be able to fire more than once" 

This reverts commit 66236c64.

Reason for revert: Bad and unexpected performance regressions were
introduced with this change. It is possible to incur an infinite loop
of loading a <link> and firing the load event, if the onload function
tampers with the `rel` attribute, as is sometimes done with preload links.
This may prevent a window's load event from ever being fired. Mitigating
problems like this needs to more looking into before this change can land
apparently.

Performance regression bugs: https://crbug.com/927427, https://crbug.com/928796, https://crbug.com/929153, https://crbug.com/929265. The original bug (https://crbug.com/922618) has been re-opened to track future work for this.

Original change's description:
> Link element events should be able to fire more than once
>
> As per spec https://html.spec.whatwg.org/multipage/links.html#link-type-stylesheet,
> the <link> element can have its load and error events fire multiple times,
> per resource it loads.
>
> Bug: 922618
> Change-Id: Ifc9ade076e119d5cf9f4eaf656c6ea7c1deb0ba9
> Reviewed-on: https://chromium-review.googlesource.com/c/1423601
> Reviewed-by: Kouhei Ueno <kouhei@chromium.org>
> Reviewed-by: Yutaka Hirano <yhirano@chromium.org>
> Commit-Queue: Dominic Farolino <domfarolino@gmail.com>
> Cr-Commit-Position: refs/heads/master@{#628010}

TBR=yhirano@chromium.org,kouhei@chromium.org,domfarolino@gmail.com

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 922618
Change-Id: I0affe5a242bf472743d11a4905a630da7895cfca
Reviewed-on: https://chromium-review.googlesource.com/c/1457745Reviewed-by: default avatarYutaka Hirano <yhirano@chromium.org>
Reviewed-by: default avatarDominic Farolino <domfarolino@gmail.com>
Commit-Queue: Dominic Farolino <domfarolino@gmail.com>
Cr-Commit-Position: refs/heads/master@{#629956}
parent ed18a48c
Hide whitespace changes
Inline Side-by-side
Showing with 5 additions and 42 deletions
third_party/blink/renderer/core/html/link_style.cc
View file @ e85090e8
...@@ -45,6 +45,7 @@ LinkStyle::LinkStyle(HTMLLinkElement* owner) ...@@ -45,6 +45,7 @@ LinkStyle::LinkStyle(HTMLLinkElement* owner)
disabled_state_(kUnset), disabled_state_(kUnset),
pending_sheet_type_(kNone), pending_sheet_type_(kNone),
loading_(false), loading_(false),
fired_load_(false),
loaded_sheet_(false) {} loaded_sheet_(false) {}
LinkStyle::~LinkStyle() = default; LinkStyle::~LinkStyle() = default;
...@@ -144,9 +145,12 @@ bool LinkStyle::SheetLoaded() { ...@@ -144,9 +145,12 @@ bool LinkStyle::SheetLoaded() {
void LinkStyle::NotifyLoadedSheetAndAllCriticalSubresources( void LinkStyle::NotifyLoadedSheetAndAllCriticalSubresources(
Node::LoadedSheetErrorStatus error_status) { Node::LoadedSheetErrorStatus error_status) {
if (fired_load_)
return;
loaded_sheet_ = (error_status == Node::kNoErrorLoadingSubresource); loaded_sheet_ = (error_status == Node::kNoErrorLoadingSubresource);
if (owner_) if (owner_)
owner_->ScheduleEvent(); owner_->ScheduleEvent();
fired_load_ = true;
} }
void LinkStyle::StartLoadingDynamicSheet() { void LinkStyle::StartLoadingDynamicSheet() {
......
third_party/blink/renderer/core/html/link_style.h
View file @ e85090e8
...@@ -79,6 +79,7 @@ class LinkStyle final : public LinkResource, ResourceClient { ...@@ -79,6 +79,7 @@ class LinkStyle final : public LinkResource, ResourceClient {
PendingSheetType pending_sheet_type_; PendingSheetType pending_sheet_type_;
StyleEngineContext style_engine_context_; StyleEngineContext style_engine_context_;
bool loading_; bool loading_;
bool fired_load_;
bool loaded_sheet_; bool loaded_sheet_;
}; };
......
third_party/blink/web_tests/external/wpt/html/semantics/document-metadata/the-link-element/link-multiple-error-events.html deleted 100644 → 0
View file @ ed18a48c
<!DOCTYPE html>
<link rel="author" title="Dom Farolino" href="mailto:dom@chromium.org">
<link rel="help" href="https://html.spec.whatwg.org/multipage/#the-link-element">
<script src="/resources/testharness.js"></script>
<script src="/resources/testharnessreport.js"></script>
<link id=link rel=stylesheet id=style_test
onload="t.unreached_func('Sheet should fail to load')">
<script>
var t = async_test("Check if the <link>'s error event fires for each style " +
"sheet it fails to load");
link.onerror = t.step_func(() => {
link.onerror = t.step_func_done(() => {});
link.href = 'nonexistent.css?second';
});
link.href = 'nonexistent.css?first';
</script>
</head>
</html>
third_party/blink/web_tests/external/wpt/html/semantics/document-metadata/the-link-element/link-multiple-load-events.html deleted 100644 → 0
View file @ ed18a48c
<!DOCTYPE html>
<link rel="author" title="Dom Farolino" href="mailto:dom@chromium.org">
<link rel="help" href="https://html.spec.whatwg.org/multipage/#the-link-element">
<script src="/resources/testharness.js"></script>
<script src="/resources/testharnessreport.js"></script>
<link id=link rel=stylesheet id=style_test
onerror="t.unreached_func('Sheet should load successfully')">
<script>
var t = async_test("Check if the <link>'s load event fires for each style " +
"sheet it loads");
link.onload = t.step_func(() => {
link.onload = t.step_func_done(() => {});
link.href = 'style.css?second';
});
link.href = 'style.css?first';
</script>
</head>
</html>
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment