|
|
DescriptionMake CSSFontFace::setLoadStatus post a task
Setting the load status to LOADED results in the promise associated with the FontFace "loaded" property being resolved synchronously. The promise is resolved with a loaded FontFace object. This could result in script being executed in a forbidden scope.
Posting a task when the status is ERROR/LOADED prevents the promise from being resolved inside a forbidden scope and matches the spec ("When the load operation completes, successfully or not, queue a task to run the following steps synchronously", see https://drafts.csswg.org/css-font-loading/#font-face-load).
BUG=663476
Review-Url: https://codereview.chromium.org/2610593002
Cr-Commit-Position: refs/heads/master@{#443596}
Committed: https://chromium.googlesource.com/chromium/src/+/76b781d16735162ce3304f95c2749742185e12f1
Patch Set 1 #Patch Set 2 : Fix affected layout test #Patch Set 3 : Rebase #Patch Set 4 : Keep track of load status in CSSFontFace #Patch Set 5 : Rebase #Patch Set 6 : Trying again #Patch Set 7 : Change loading logic for fonts from array buffers #
Total comments: 4
Patch Set 8 : Revert changes to CSSFontFace and post task in FontFace #
Total comments: 8
Patch Set 9 : Run callbacks in a task #
Total comments: 2
Patch Set 10 : Add comment #
Messages
Total messages: 51 (35 generated)
The CQ bit was checked by adithyas@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by adithyas@chromium.org
The CQ bit was checked by adithyas@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Make FontFace::setLoadStatus post task BUG= ========== to ========== Make CSSFontFace::setLoadStatus post a task Setting the load status to LOADED results in the promise associated with the FontFace "loaded" property being resolved synchronously. The promise is resolved with a loaded FontFace object. This could result in script being executed synchronously (if FontFace is made thenable, i.e. a "then" property is added to its prototype). Since it is possible for this function to be called during layout, script can be executed in a forbidden scope. Posting a task when the status is ERROR/LOADED prevents the promise from being resolved inside a forbidden scope and matches the spec ("When the load operation completes, successfully or not, queue a task to run the following steps synchronously", see https://drafts.csswg.org/css-font-loading/#font-face-load). BUG=663476 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by adithyas@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by adithyas@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by adithyas@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by adithyas@chromium.org
The CQ bit was checked by adithyas@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Make CSSFontFace::setLoadStatus post a task Setting the load status to LOADED results in the promise associated with the FontFace "loaded" property being resolved synchronously. The promise is resolved with a loaded FontFace object. This could result in script being executed synchronously (if FontFace is made thenable, i.e. a "then" property is added to its prototype). Since it is possible for this function to be called during layout, script can be executed in a forbidden scope. Posting a task when the status is ERROR/LOADED prevents the promise from being resolved inside a forbidden scope and matches the spec ("When the load operation completes, successfully or not, queue a task to run the following steps synchronously", see https://drafts.csswg.org/css-font-loading/#font-face-load). BUG=663476 ========== to ========== Make CSSFontFace::setLoadStatus post a task Setting the load status to LOADED results in the promise associated with the FontFace "loaded" property being resolved synchronously. The promise is resolved with a loaded FontFace object. This could result in script being executed in a forbidden scope. Posting a task when the status is ERROR/LOADED prevents the promise from being resolved inside a forbidden scope and matches the spec ("When the load operation completes, successfully or not, queue a task to run the following steps synchronously", see https://drafts.csswg.org/css-font-loading/#font-face-load). BUG=663476 ==========
adithyas@chromium.org changed reviewers: + jbroman@chromium.org, ksakamoto@chromium.org
ksakamoto@: This patch tries to fix potential script execution inside a script forbidden scope caused by a FontFace promise being resolved during layout. I've tried to explained this patch in more detail in the description, could you please take a look?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
one nit; leaving the CSSFontFace details to ksakamoto https://codereview.chromium.org/2610593002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/CSSFontFace.cpp (right): https://codereview.chromium.org/2610593002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/CSSFontFace.cpp:192: if (!m_segmentedFontFace) nit: it's a little confusing that stuff that only happens in the FontFace::Loading case is both here and above (in the if statement). Would you mind restructuring to put them together? Either like this: if (newStatus != FontFace::Loading) { // post task return; } m_fontFace->setLoadStatus(newStatus); // segmented font face stuff or like this: if (newStatus == FontFace::Loading) { m_fontFace->setLoadStatus(newStatus); // segmented font face stuff } else { // post task } or something similar
https://codereview.chromium.org/2610593002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/FontFace.cpp (right): https://codereview.chromium.org/2610593002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/FontFace.cpp:353: if (m_status == Loaded || m_status == Error) { Having load status in both FontFace and CSSFontFace (with slightly different meanings) is unfortunate. Instead, can we make just the promise resolution asynchronous? (i.e. delay the body of this if-statement using postTask)
https://codereview.chromium.org/2610593002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/FontFace.cpp (right): https://codereview.chromium.org/2610593002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/FontFace.cpp:353: if (m_status == Loaded || m_status == Error) { On 2017/01/06 at 07:05:47, Kunihiko Sakamoto wrote: > Having load status in both FontFace and CSSFontFace (with slightly different meanings) is unfortunate. > Instead, can we make just the promise resolution asynchronous? (i.e. delay the body of this if-statement using postTask) SGTM, but this would mean that there would be a delay between the loadStatus being set and the promise being resolved. Is that okay?
The CQ bit was checked by adithyas@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2610593002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/FontFace.cpp (right): https://codereview.chromium.org/2610593002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/FontFace.cpp:353: if (m_status == Loaded || m_status == Error) { On 2017/01/06 14:56:13, adithyas wrote: > On 2017/01/06 at 07:05:47, Kunihiko Sakamoto wrote: > > Having load status in both FontFace and CSSFontFace (with slightly different > meanings) is unfortunate. > > Instead, can we make just the promise resolution asynchronous? (i.e. delay the > body of this if-statement using postTask) > > SGTM, but this would mean that there would be a delay between the loadStatus > being set and the promise being resolved. Is that okay? Yeah ideally we should change the status inside the posted task, but in various places we're assuming that m_status is immediately updated when loaded. This is slightly different from what the spec says, but I don't think this will cause problems in practice. https://codereview.chromium.org/2610593002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/FontFace.cpp (right): https://codereview.chromium.org/2610593002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/FontFace.cpp:364: m_loadedProperty->reject(m_error.get()); Need to post a task for rejection too? https://codereview.chromium.org/2610593002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/FontFace.cpp:371: callbacks[i]->notifyLoaded(this); This may resolve a promise (that is created by FontFaceSet.load()). So I think we should delay the body of if (m_status == Loaded || m_status == Error) { ... } entirely.
https://codereview.chromium.org/2610593002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/FontFace.cpp (right): https://codereview.chromium.org/2610593002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/FontFace.cpp:364: m_loadedProperty->reject(m_error.get()); On 2017/01/10 at 06:02:12, Kunihiko Sakamoto wrote: > Need to post a task for rejection too? Rejection is always asynchronous so we don't have to post a task. https://codereview.chromium.org/2610593002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/FontFace.cpp:371: callbacks[i]->notifyLoaded(this); On 2017/01/10 at 06:02:12, Kunihiko Sakamoto wrote: > This may resolve a promise (that is created by FontFaceSet.load()). > > So I think we should delay the body of > if (m_status == Loaded || m_status == Error) { ... } > entirely. FontFaceSet uses an AsyncMethodRunner to resolve the promise/fire a done event, so that's already asynchronous.
https://codereview.chromium.org/2610593002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/FontFace.cpp (right): https://codereview.chromium.org/2610593002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/FontFace.cpp:364: m_loadedProperty->reject(m_error.get()); On 2017/01/10 17:52:46, adithyas wrote: > On 2017/01/10 at 06:02:12, Kunihiko Sakamoto wrote: > > Need to post a task for rejection too? > > Rejection is always asynchronous so we don't have to post a task. Ah interesting, didn't know that. https://codereview.chromium.org/2610593002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/FontFace.cpp:371: callbacks[i]->notifyLoaded(this); On 2017/01/10 17:52:46, adithyas wrote: > On 2017/01/10 at 06:02:12, Kunihiko Sakamoto wrote: > > This may resolve a promise (that is created by FontFaceSet.load()). > > > > So I think we should delay the body of > > if (m_status == Loaded || m_status == Error) { ... } > > entirely. > > FontFaceSet uses an AsyncMethodRunner to resolve the promise/fire a done event, > so that's already asynchronous. FontFaceSet::notifyLoaded() is OK, but LoadFontPromiseResolver::notifyLoaded() resolves m_resolver synchronously.
https://codereview.chromium.org/2610593002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/FontFace.cpp (right): https://codereview.chromium.org/2610593002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/FontFace.cpp:364: m_loadedProperty->reject(m_error.get()); On 2017/01/11 at 05:25:45, Kunihiko Sakamoto wrote: > On 2017/01/10 17:52:46, adithyas wrote: > > On 2017/01/10 at 06:02:12, Kunihiko Sakamoto wrote: > > > Need to post a task for rejection too? > > > > Rejection is always asynchronous so we don't have to post a task. > > Ah interesting, didn't know that. Technically, resolution is also always asynchronous (the callback is executed in a separate microtask). But resolving promises involves accessing the 'then' property of the resolved value, so a getter for 'then' could be executed synchronously. https://codereview.chromium.org/2610593002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/FontFace.cpp:371: callbacks[i]->notifyLoaded(this); On 2017/01/11 at 05:25:45, Kunihiko Sakamoto wrote: > On 2017/01/10 17:52:46, adithyas wrote: > > On 2017/01/10 at 06:02:12, Kunihiko Sakamoto wrote: > > > This may resolve a promise (that is created by FontFaceSet.load()). > > > > > > So I think we should delay the body of > > > if (m_status == Loaded || m_status == Error) { ... } > > > entirely. > > > > FontFaceSet uses an AsyncMethodRunner to resolve the promise/fire a done event, > > so that's already asynchronous. > > FontFaceSet::notifyLoaded() is OK, but LoadFontPromiseResolver::notifyLoaded() resolves m_resolver synchronously. Ahh okay, I've changed it to run the callbacks in a task.
lgtm Thank you for the fix!
The CQ bit was checked by adithyas@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
lgtm https://codereview.chromium.org/2610593002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/FontFace.cpp (right): https://codereview.chromium.org/2610593002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/FontFace.cpp:357: if (m_status == Loaded) { Would you mind adding a comment explaining why this is necessary for resolve and not reject? Ideally we would treat both the same, so an explanation of why they're different would be breat.
https://codereview.chromium.org/2610593002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/FontFace.cpp (right): https://codereview.chromium.org/2610593002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/FontFace.cpp:357: if (m_status == Loaded) { On 2017/01/12 at 18:40:46, jbroman wrote: > Would you mind adding a comment explaining why this is necessary for resolve and not reject? Ideally we would treat both the same, so an explanation of why they're different would be breat. Added a comment.
lgtm
The CQ bit was checked by adithyas@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ksakamoto@chromium.org Link to the patchset: https://codereview.chromium.org/2610593002/#ps180001 (title: "Add comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 180001, "attempt_start_ts": 1484324207140940, "parent_rev": "964dc83216c79bc4495c3acfc956276146d9e1df", "commit_rev": "76b781d16735162ce3304f95c2749742185e12f1"}
Message was sent while issue was closed.
Description was changed from ========== Make CSSFontFace::setLoadStatus post a task Setting the load status to LOADED results in the promise associated with the FontFace "loaded" property being resolved synchronously. The promise is resolved with a loaded FontFace object. This could result in script being executed in a forbidden scope. Posting a task when the status is ERROR/LOADED prevents the promise from being resolved inside a forbidden scope and matches the spec ("When the load operation completes, successfully or not, queue a task to run the following steps synchronously", see https://drafts.csswg.org/css-font-loading/#font-face-load). BUG=663476 ========== to ========== Make CSSFontFace::setLoadStatus post a task Setting the load status to LOADED results in the promise associated with the FontFace "loaded" property being resolved synchronously. The promise is resolved with a loaded FontFace object. This could result in script being executed in a forbidden scope. Posting a task when the status is ERROR/LOADED prevents the promise from being resolved inside a forbidden scope and matches the spec ("When the load operation completes, successfully or not, queue a task to run the following steps synchronously", see https://drafts.csswg.org/css-font-loading/#font-face-load). BUG=663476 Review-Url: https://codereview.chromium.org/2610593002 Cr-Commit-Position: refs/heads/master@{#443596} Committed: https://chromium.googlesource.com/chromium/src/+/76b781d16735162ce3304f95c274... ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001) as https://chromium.googlesource.com/chromium/src/+/76b781d16735162ce3304f95c274... |