[PEPC] Fix crash when a PermissionElement is made from a shutdown reference. [chromium/src : main]
Thomas Nguyen (Gerrit)
Thomas Nguyen added 1 comment![]( "Open in Gerrit")
return GetExecutionContext()->GetTaskRunner(TaskType::kInternalDefault);I think we need to protect some other places of dereferencing the execution context in this file.
Related details
- Joey Arhar
- Ravjit Uppal
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Balazs Engedy (Gerrit)
Balazs Engedy added 1 comment![]( "Open in Gerrit")
return GetExecutionContext()->GetTaskRunner(TaskType::kInternalDefault);I think we need to protect some other places of dereferencing the execution context in this file.
My suggestion: Let's make _this change_ nevertheless to bring us in line with other HTMLElement subclass implementations, *and* audit if other places need hardening too.
For the purposes of this CL, I would propose that we limit the scope of auditing to cover exactly those methods/behaviors that we can reasonably expect to happen on a HTMLPermissionElement that is created for an already shut down document. This way, we ensure that we land a CL that actually prevents a crash in the scenario we were seeing in the crash report, and we are not just kicking the can further down the road and crashing one line later (i.e. successful construction of the element, but then the next thing done on it crashes).
I didn't immediately find any safeguards that prevents the element from being inserted into the same, already shut-down document, I'm hoping I just missed it, but then we would need to update InsertedInto() as well. I am assuming that at least a shut-down document cannot have a layout tree attached, the other read-only attributes do not need an execution context.
In the <geolocation> element there are some more complex reactions to setting attributes like `autolocate` and `watch`, it wasn't immediately clear to me how those behave on a null execution context.
Separately from that work, there can be other fun edge cases, like a warmed up HTMLPermissionElement from an active document being removed and reinsterted into, and adopted by, a shut-down document. Then vice versa.
It sounds to me that there must be some existing patterns that other elements implement so that this does not get out of hand and result in us changing almost always that the executioncontext is non-null.
Related details
- Joey Arhar
- Ravjit Uppal
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thomas Nguyen (Gerrit)
Thomas Nguyen added 1 comment![]( "Open in Gerrit")
return GetExecutionContext()->GetTaskRunner(TaskType::kInternalDefault);Balazs EngedyI think we need to protect some other places of dereferencing the execution context in this file.
My suggestion: Let's make _this change_ nevertheless to bring us in line with other HTMLElement subclass implementations, *and* audit if other places need hardening too.
For the purposes of this CL, I would propose that we limit the scope of auditing to cover exactly those methods/behaviors that we can reasonably expect to happen on a HTMLPermissionElement that is created for an already shut down document. This way, we ensure that we land a CL that actually prevents a crash in the scenario we were seeing in the crash report, and we are not just kicking the can further down the road and crashing one line later (i.e. successful construction of the element, but then the next thing done on it crashes).
I didn't immediately find any safeguards that prevents the element from being inserted into the same, already shut-down document, I'm hoping I just missed it, but then we would need to update InsertedInto() as well. I am assuming that at least a shut-down document cannot have a layout tree attached, the other read-only attributes do not need an execution context.
In the <geolocation> element there are some more complex reactions to setting attributes like `autolocate` and `watch`, it wasn't immediately clear to me how those behave on a null execution context.
Separately from that work, there can be other fun edge cases, like a warmed up HTMLPermissionElement from an active document being removed and reinsterted into, and adopted by, a shut-down document. Then vice versa.
It sounds to me that there must be some existing patterns that other elements implement so that this does not get out of hand and result in us changing almost always that the executioncontext is non-null.
It is a good point indeed. Changes to the execution context can lead to complications, and we should to prevent such cases. This pattern of re-invalidating the context when elements are moved within a document is common in other elements; we noted as a backlog by may have previously underestimated this.
Regarding the hardening, just in a safe side, we might consider scenarios such as during the triggering of ParseAttribute, which may ultimately involve dereferencing the execution context.
https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/html/html_permission_element.cc;l=806;drc=bf712ec1a13783224debb691ba88ad5c15b93194;bpv=1;bpt=1
(just tracing the code but I have not verify myself)
Joey Arhar (Gerrit)
Joey Arhar added 2 comments![]( "Open in Gerrit")
Could this be written as a WPT crash test instead?
return GetDocument().GetTaskRunner(TaskType::kInternalDefault);Interesting, I didn't know that an element's ExecutionContext could be different from its document's ExecutionContext
Related details
- Ravjit Uppal
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Balazs Engedy (Gerrit)
Balazs Engedy added 2 comments![]( "Open in Gerrit")
return GetExecutionContext()->GetTaskRunner(TaskType::kInternalDefault);Balazs EngedyI think we need to protect some other places of dereferencing the execution context in this file.
Thomas NguyenMy suggestion: Let's make _this change_ nevertheless to bring us in line with other HTMLElement subclass implementations, *and* audit if other places need hardening too.
For the purposes of this CL, I would propose that we limit the scope of auditing to cover exactly those methods/behaviors that we can reasonably expect to happen on a HTMLPermissionElement that is created for an already shut down document. This way, we ensure that we land a CL that actually prevents a crash in the scenario we were seeing in the crash report, and we are not just kicking the can further down the road and crashing one line later (i.e. successful construction of the element, but then the next thing done on it crashes).
I didn't immediately find any safeguards that prevents the element from being inserted into the same, already shut-down document, I'm hoping I just missed it, but then we would need to update InsertedInto() as well. I am assuming that at least a shut-down document cannot have a layout tree attached, the other read-only attributes do not need an execution context.
In the <geolocation> element there are some more complex reactions to setting attributes like `autolocate` and `watch`, it wasn't immediately clear to me how those behave on a null execution context.
Separately from that work, there can be other fun edge cases, like a warmed up HTMLPermissionElement from an active document being removed and reinsterted into, and adopted by, a shut-down document. Then vice versa.
It sounds to me that there must be some existing patterns that other elements implement so that this does not get out of hand and result in us changing almost always that the executioncontext is non-null.
It is a good point indeed. Changes to the execution context can lead to complications, and we should to prevent such cases. This pattern of re-invalidating the context when elements are moved within a document is common in other elements; we noted as a backlog by may have previously underestimated this.
Regarding the hardening, just in a safe side, we might consider scenarios such as during the triggering of ParseAttribute, which may ultimately involve dereferencing the execution context.
https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/html/html_permission_element.cc;l=806;drc=bf712ec1a13783224debb691ba88ad5c15b93194;bpv=1;bpt=1
(just tracing the code but I have not verify myself)
Naive question: is `ParseAttribute` also called when you set it from programmatically from JS? It sounds like it would be good to extend the test to not just create a <geolocation> element, but to set all the attributes on it that developers would normally set, and then insert it into the shutdown document (is there some event that we can wait on to ensure that happens)?
return GetDocument().GetTaskRunner(TaskType::kInternalDefault);Interesting, I didn't know that an element's ExecutionContext could be different from its document's ExecutionContext
It's actually not a different ExecutionContext, but the document's taskrunner getter returns a fallback in case the execution context is null.
Thomas Nguyen (Gerrit)
Thomas Nguyen added 1 comment![]( "Open in Gerrit")
return GetExecutionContext()->GetTaskRunner(TaskType::kInternalDefault);Balazs EngedyI think we need to protect some other places of dereferencing the execution context in this file.
Thomas NguyenMy suggestion: Let's make _this change_ nevertheless to bring us in line with other HTMLElement subclass implementations, *and* audit if other places need hardening too.
For the purposes of this CL, I would propose that we limit the scope of auditing to cover exactly those methods/behaviors that we can reasonably expect to happen on a HTMLPermissionElement that is created for an already shut down document. This way, we ensure that we land a CL that actually prevents a crash in the scenario we were seeing in the crash report, and we are not just kicking the can further down the road and crashing one line later (i.e. successful construction of the element, but then the next thing done on it crashes).
I didn't immediately find any safeguards that prevents the element from being inserted into the same, already shut-down document, I'm hoping I just missed it, but then we would need to update InsertedInto() as well. I am assuming that at least a shut-down document cannot have a layout tree attached, the other read-only attributes do not need an execution context.
In the <geolocation> element there are some more complex reactions to setting attributes like `autolocate` and `watch`, it wasn't immediately clear to me how those behave on a null execution context.
Separately from that work, there can be other fun edge cases, like a warmed up HTMLPermissionElement from an active document being removed and reinsterted into, and adopted by, a shut-down document. Then vice versa.
It sounds to me that there must be some existing patterns that other elements implement so that this does not get out of hand and result in us changing almost always that the executioncontext is non-null.
Balazs EngedyIt is a good point indeed. Changes to the execution context can lead to complications, and we should to prevent such cases. This pattern of re-invalidating the context when elements are moved within a document is common in other elements; we noted as a backlog by may have previously underestimated this.
Regarding the hardening, just in a safe side, we might consider scenarios such as during the triggering of ParseAttribute, which may ultimately involve dereferencing the execution context.
https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/html/html_permission_element.cc;l=806;drc=bf712ec1a13783224debb691ba88ad5c15b93194;bpv=1;bpt=1
(just tracing the code but I have not verify myself)
Naive question: is `ParseAttribute` also called when you set it from programmatically from JS? It sounds like it would be good to extend the test to not just create a <geolocation> element, but to set all the attributes on it that developers would normally set, and then insert it into the shutdown document (is there some event that we can wait on to ensure that happens)?
Agree, we should do that (as I saw precedent case of checking null ExecutionContext in ParseAttribute) such as in form element.
Ravjit Uppal (Gerrit)
Ravjit Uppal added 2 comments![]( "Open in Gerrit")
Could this be written as a WPT crash test instead?
Done
return GetExecutionContext()->GetTaskRunner(TaskType::kInternalDefault);Balazs EngedyI think we need to protect some other places of dereferencing the execution context in this file.
Thomas NguyenMy suggestion: Let's make _this change_ nevertheless to bring us in line with other HTMLElement subclass implementations, *and* audit if other places need hardening too.
For the purposes of this CL, I would propose that we limit the scope of auditing to cover exactly those methods/behaviors that we can reasonably expect to happen on a HTMLPermissionElement that is created for an already shut down document. This way, we ensure that we land a CL that actually prevents a crash in the scenario we were seeing in the crash report, and we are not just kicking the can further down the road and crashing one line later (i.e. successful construction of the element, but then the next thing done on it crashes).
I didn't immediately find any safeguards that prevents the element from being inserted into the same, already shut-down document, I'm hoping I just missed it, but then we would need to update InsertedInto() as well. I am assuming that at least a shut-down document cannot have a layout tree attached, the other read-only attributes do not need an execution context.
In the <geolocation> element there are some more complex reactions to setting attributes like `autolocate` and `watch`, it wasn't immediately clear to me how those behave on a null execution context.
Separately from that work, there can be other fun edge cases, like a warmed up HTMLPermissionElement from an active document being removed and reinsterted into, and adopted by, a shut-down document. Then vice versa.
It sounds to me that there must be some existing patterns that other elements implement so that this does not get out of hand and result in us changing almost always that the executioncontext is non-null.
Balazs EngedyIt is a good point indeed. Changes to the execution context can lead to complications, and we should to prevent such cases. This pattern of re-invalidating the context when elements are moved within a document is common in other elements; we noted as a backlog by may have previously underestimated this.
Regarding the hardening, just in a safe side, we might consider scenarios such as during the triggering of ParseAttribute, which may ultimately involve dereferencing the execution context.
https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/html/html_permission_element.cc;l=806;drc=bf712ec1a13783224debb691ba88ad5c15b93194;bpv=1;bpt=1
(just tracing the code but I have not verify myself)
Thomas NguyenNaive question: is `ParseAttribute` also called when you set it from programmatically from JS? It sounds like it would be good to extend the test to not just create a <geolocation> element, but to set all the attributes on it that developers would normally set, and then insert it into the shutdown document (is there some event that we can wait on to ensure that happens)?
Agree, we should do that (as I saw precedent case of checking null ExecutionContext in ParseAttribute) such as in form element.
We do get a crash if we try to insert the element into a already shutdown element. I added a null check in `InsertedInto` and extended the test.
Related details
- Balazs Engedy
- Joey Arhar
- Thomas Nguyen
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joey Arhar (Gerrit)
Joey Arhar added 2 comments![]( "Open in Gerrit")
return GetDocument().GetTaskRunner(TaskType::kInternalDefault);Balazs EngedyInteresting, I didn't know that an element's ExecutionContext could be different from its document's ExecutionContext
It's actually not a different ExecutionContext, but the document's taskrunner getter returns a fallback in case the execution context is null.
Acknowledged
crashtests don't need to use testharness and end with crash.html: https://web-platform-tests.org/writing-tests/crashtest.html
you can use the class=test-wait described there to replace the t.step_wait from testharness
Related details
- Balazs Engedy
- Ravjit Uppal
- Thomas Nguyen
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ravjit Uppal (Gerrit)
Ravjit Uppal voted and added 2 comments![]( "Open in Gerrit")
Votes added by Ravjit Uppal
| Auto-Submit | +1 |
| Commit-Queue | +1 |
2 comments
return GetExecutionContext()->GetTaskRunner(TaskType::kInternalDefault);Balazs EngedyI think we need to protect some other places of dereferencing the execution context in this file.
Thomas NguyenMy suggestion: Let's make _this change_ nevertheless to bring us in line with other HTMLElement subclass implementations, *and* audit if other places need hardening too.
For the purposes of this CL, I would propose that we limit the scope of auditing to cover exactly those methods/behaviors that we can reasonably expect to happen on a HTMLPermissionElement that is created for an already shut down document. This way, we ensure that we land a CL that actually prevents a crash in the scenario we were seeing in the crash report, and we are not just kicking the can further down the road and crashing one line later (i.e. successful construction of the element, but then the next thing done on it crashes).
I didn't immediately find any safeguards that prevents the element from being inserted into the same, already shut-down document, I'm hoping I just missed it, but then we would need to update InsertedInto() as well. I am assuming that at least a shut-down document cannot have a layout tree attached, the other read-only attributes do not need an execution context.
In the <geolocation> element there are some more complex reactions to setting attributes like `autolocate` and `watch`, it wasn't immediately clear to me how those behave on a null execution context.
Separately from that work, there can be other fun edge cases, like a warmed up HTMLPermissionElement from an active document being removed and reinsterted into, and adopted by, a shut-down document. Then vice versa.
It sounds to me that there must be some existing patterns that other elements implement so that this does not get out of hand and result in us changing almost always that the executioncontext is non-null.
Balazs EngedyIt is a good point indeed. Changes to the execution context can lead to complications, and we should to prevent such cases. This pattern of re-invalidating the context when elements are moved within a document is common in other elements; we noted as a backlog by may have previously underestimated this.
Regarding the hardening, just in a safe side, we might consider scenarios such as during the triggering of ParseAttribute, which may ultimately involve dereferencing the execution context.
https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/html/html_permission_element.cc;l=806;drc=bf712ec1a13783224debb691ba88ad5c15b93194;bpv=1;bpt=1
(just tracing the code but I have not verify myself)
Thomas NguyenNaive question: is `ParseAttribute` also called when you set it from programmatically from JS? It sounds like it would be good to extend the test to not just create a <geolocation> element, but to set all the attributes on it that developers would normally set, and then insert it into the shutdown document (is there some event that we can wait on to ensure that happens)?
Ravjit UppalAgree, we should do that (as I saw precedent case of checking null ExecutionContext in ParseAttribute) such as in form element.
We do get a crash if we try to insert the element into a already shutdown element. I added a null check in `InsertedInto` and extended the test.
Done
crashtests don't need to use testharness and end with crash.html: https://web-platform-tests.org/writing-tests/crashtest.html
you can use the class=test-wait described there to replace the t.step_wait from testharness
- Balazs Engedy
- Joey Arhar
- Thomas Nguyen
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joey Arhar (Gerrit)
Joey Arhar added 1 comment![]( "Open in Gerrit")
setTimeout(() => {Maybe instead of using setTimeout since that's disallowed in wpt, you could wait for a load event to be fired on the iframe? and if thats not enough, maybe add a couple requestAnimationFrames?
Related details
- Balazs Engedy
- Ravjit Uppal
- Thomas Nguyen
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ravjit Uppal (Gerrit)
Ravjit Uppal voted and added 1 comment![]( "Open in Gerrit")
Votes added by Ravjit Uppal
| Auto-Submit | +1 |
1 comment
Maybe instead of using setTimeout since that's disallowed in wpt, you could wait for a load event to be fired on the iframe? and if thats not enough, maybe add a couple requestAnimationFrames?
- Balazs Engedy
- Joey Arhar
- Thomas Nguyen
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joey Arhar (Gerrit)
Joey Arhar voted![]( "Open in Gerrit")
- Balazs Engedy
- Ravjit Uppal
- Thomas Nguyen
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Blink W3C Test Autoroller (Gerrit)
Message from Blink W3C Test Autoroller![]( "Open in 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/web-platform-tests/wpt/pull/57392.
When this CL lands, the bot will automatically merge the PR on GitHub if the required GitHub checks pass; otherwise, ecosystem-infra@ team will triage the failures and may contact you.
WPT Export docs:
https://chromium.googlesource.com/chromium/src/+/main/docs/testing/web_platform_tests.md#Automatic-export-process
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
[PEPC] Fix crash when a PermissionElement is made from a shutdown reference.
- M third_party/blink/renderer/core/html/html_permission_element.cc
- A third_party/blink/web_tests/external/wpt/html/semantics/permission-element/geolocation-element/iframe-use-after-free-crash.html
Code-Review: +1 by Joey Arhar
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Blink W3C Test Autoroller (Gerrit)
Message from Blink W3C Test Autoroller![]( "Open in Gerrit")
The WPT PR for this CL has been merged upstream! https://github.com/web-platform-tests/wpt/pull/57392
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |