|
|
Created:
5 years, 2 months ago by kouhei (in TOK) Modified:
5 years, 2 months ago CC:
chromium-reviews, blink-reviews, dglazkov+blink, blink-reviews-html_chromium.org, kinuko+watch Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionCrash fix: Avoid using stale HTMLToken after tree construction
BUG=526286, 468406
Committed: https://crrev.com/944deb3adba8aae2a1fc793937a93a051f08e7d7
Cr-Commit-Position: refs/heads/master@{#352035}
Patch Set 1 #
Total comments: 2
Patch Set 2 : fix logic #Patch Set 3 : move m_framesetOk #Patch Set 4 : add comments #Patch Set 5 : add tests #Patch Set 6 : add TODO about mutation #
Messages
Total messages: 29 (8 generated)
kouhei@chromium.org changed reviewers: + kinuko@chromium.org, yoav@yoav.ws
I failed to create a reliable repro test case for this. I'd like to rely on ClusterFuzz for regression catching.
On 2015/09/29 03:46:18, kouhei (catching-up) wrote: > I failed to create a reliable repro test case for this. I'd like to rely on > ClusterFuzz for regression catching. Can you please give me permissions for the bugs in question, so I can understand the reasons for this change? :)
The CQ bit was checked by kouhei@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1372343002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1372343002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
https://codereview.chromium.org/1372343002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/parser/HTMLTreeBuilder.cpp (right): https://codereview.chromium.org/1372343002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/parser/HTMLTreeBuilder.cpp:799: m_framesetOk = disableFrameset; Can you detail what you suspect is happening in the minimized test case? What makes you think this change will give us a better picture of what's going on? A few more questions about this specific change: * We could just get rid of disableFrameset and assign the expression directly to m_framesetOk, right? * IIUC, that change will cause a behavior change since we'd be now changing the state of m_framesetOk - When disableFrameset is true, m_framesetOk will be true when previously it was false - When disableFrameset is false, m_framesetOk will be false when previously it maintained its previous state. * Is that behavior change intentional? If so, can we test it? (I'm guessing it is not since I don't see framesets on the test case in https://code.google.com/p/chromium/issues/detail?id=468406 )
https://codereview.chromium.org/1372343002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/parser/HTMLTreeBuilder.cpp (right): https://codereview.chromium.org/1372343002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/parser/HTMLTreeBuilder.cpp:799: m_framesetOk = disableFrameset; On 2015/09/29 07:12:42, Yoav Weiss wrote: > Can you detail what you suspect is happening in the minimized test case? What > makes you think this change will give us a better picture of what's going on? > > A few more questions about this specific change: > * We could just get rid of disableFrameset and assign the expression directly to > m_framesetOk, right? > * IIUC, that change will cause a behavior change since we'd be now changing the > state of m_framesetOk > - When disableFrameset is true, m_framesetOk will be true when previously it > was false > - When disableFrameset is false, m_framesetOk will be false when previously it > maintained its previous state. > * Is that behavior change intentional? If so, can we test it? (I'm guessing it > is not since I don't see framesets on the test case in > https://code.google.com/p/chromium/issues/detail?id=468406 ) Thanks for the catch. This wasn't intentional. Let me try to write a test for this again... and first I need to understand why this logic is here in the first place.
OK. I have better understanding of this bug now. Please review code changes in PS#4. I'll try to add a repro test tomorrow. > On 2015/09/29 07:12:42, Yoav Weiss wrote: > > Can you detail what you suspect is happening in the minimized test case? What > > makes you think this change will give us a better picture of what's going on? m_tree.insertSelfClosingHTMLElement(token) may end up mutating m_attributes vector, and typeAttribute ptr captured before the call becomes stale. #0 WTF::Vector<blink::Attribute, 0ul, WTF::DefaultAllocator>::shrink (this=0x7fffcbc386a8, size=1) at ../../third_party/WebKit/Source/wtf/Vector.h:1019 #1 0x00007fffef2689ec in blink::Element::stripScriptingAttributes (this=0x95178430130, attributeVector=...) at ../../third_party/WebKit/Source/core/dom/Element.cpp:1350 #2 0x00007fffef48bb83 in blink::setAttributes (element=0x95178430130, token=0x7fffcbc38680, parserContentPolicy=blink::DisallowScriptingAndPluginContent) at ../../third_party/WebKit/Source/core/html/parser/HTMLConstructionSite.cpp:65 #3 0x00007fffef48cf1d in blink::HTMLConstructionSite::createHTMLElement (this=0x2547b7498c28, token=0x7fffcbc38680) at ../../third_party/WebKit/Source/core/html/parser/HTMLConstructionSite.cpp:757 #4 0x00007fffef48d42a in blink::HTMLConstructionSite::insertSelfClosingHTMLElement (this=0x2547b7498c28, token=0x7fffcbc38680) at ../../third_party/WebKit/Source/core/html/parser/HTMLConstructionSite.cpp:633 #5 0x00007fffef4d46cb in blink::HTMLTreeBuilder::processStartTagForInBody (this=0x2547b7498c10, token=0x7fffcbc38680) at ../../third_party/WebKit/Source/core/html/parser/HTMLTreeBuilder.cpp:805 #6 0x00007fffef4ce8fb in blink::HTMLTreeBuilder::processStartTag (this=0x2547b7498c10, token=0x7fffcbc38680) at ../../third_party/WebKit/Source/core/html/parser/HTMLTreeBuilder.cpp:1154 > > A few more questions about this specific change: > > * We could just get rid of disableFrameset and assign the expression directly > to > > m_framesetOk, right? > > * IIUC, that change will cause a behavior change since we'd be now changing > the > > state of m_framesetOk > > - When disableFrameset is true, m_framesetOk will be true when previously it > > was false > > - When disableFrameset is false, m_framesetOk will be false when previously > it > > maintained its previous state. > > * Is that behavior change intentional? If so, can we test it? (I'm guessing it > > is not since I don't see framesets on the test case in > > https://code.google.com/p/chromium/issues/detail?id=468406 ) > > Thanks for the catch. This wasn't intentional. Let me try to write a test for > this again... and first I need to understand why this logic is here in the first > place. Looks like this logic is already covered in existing layout tests: https://storage.googleapis.com/chromium-layout-test-archives/linux_chromium_r...
The CQ bit was checked by kouhei@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1372343002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1372343002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Added tests. Please take a look. On 2015/10/01 12:38:31, kouhei (catching-up) wrote: > OK. I have better understanding of this bug now. Please review code changes in > PS#4. I'll try to add a repro test tomorrow. > > > On 2015/09/29 07:12:42, Yoav Weiss wrote: > > > Can you detail what you suspect is happening in the minimized test case? > What > > > makes you think this change will give us a better picture of what's going > on? > m_tree.insertSelfClosingHTMLElement(token) may end up mutating m_attributes > vector, and typeAttribute ptr captured before the call becomes stale. > > #0 WTF::Vector<blink::Attribute, 0ul, WTF::DefaultAllocator>::shrink > (this=0x7fffcbc386a8, size=1) at > ../../third_party/WebKit/Source/wtf/Vector.h:1019 > #1 0x00007fffef2689ec in blink::Element::stripScriptingAttributes > (this=0x95178430130, attributeVector=...) at > ../../third_party/WebKit/Source/core/dom/Element.cpp:1350 > #2 0x00007fffef48bb83 in blink::setAttributes (element=0x95178430130, > token=0x7fffcbc38680, > parserContentPolicy=blink::DisallowScriptingAndPluginContent) at > ../../third_party/WebKit/Source/core/html/parser/HTMLConstructionSite.cpp:65 > #3 0x00007fffef48cf1d in blink::HTMLConstructionSite::createHTMLElement > (this=0x2547b7498c28, token=0x7fffcbc38680) at > ../../third_party/WebKit/Source/core/html/parser/HTMLConstructionSite.cpp:757 > #4 0x00007fffef48d42a in > blink::HTMLConstructionSite::insertSelfClosingHTMLElement (this=0x2547b7498c28, > token=0x7fffcbc38680) at > ../../third_party/WebKit/Source/core/html/parser/HTMLConstructionSite.cpp:633 > #5 0x00007fffef4d46cb in blink::HTMLTreeBuilder::processStartTagForInBody > (this=0x2547b7498c10, token=0x7fffcbc38680) at > ../../third_party/WebKit/Source/core/html/parser/HTMLTreeBuilder.cpp:805 > #6 0x00007fffef4ce8fb in blink::HTMLTreeBuilder::processStartTag > (this=0x2547b7498c10, token=0x7fffcbc38680) at > ../../third_party/WebKit/Source/core/html/parser/HTMLTreeBuilder.cpp:1154 > > > > A few more questions about this specific change: > > > * We could just get rid of disableFrameset and assign the expression > directly > > to > > > m_framesetOk, right? Yes, but I prefer the current code as it matches the order described in https://html.spec.whatwg.org/#parsing-main-inbody algorithm.
On 2015/10/02 08:52:37, kouhei (catching-up) wrote: > Added tests. Please take a look. > > On 2015/10/01 12:38:31, kouhei (catching-up) wrote: > > OK. I have better understanding of this bug now. Please review code changes in > > PS#4. I'll try to add a repro test tomorrow. > > > > > On 2015/09/29 07:12:42, Yoav Weiss wrote: > > > > Can you detail what you suspect is happening in the minimized test case? > > What > > > > makes you think this change will give us a better picture of what's going > > on? > > m_tree.insertSelfClosingHTMLElement(token) may end up mutating m_attributes > > vector, and typeAttribute ptr captured before the call becomes stale. > > > > #0 WTF::Vector<blink::Attribute, 0ul, WTF::DefaultAllocator>::shrink > > (this=0x7fffcbc386a8, size=1) at > > ../../third_party/WebKit/Source/wtf/Vector.h:1019 > > #1 0x00007fffef2689ec in blink::Element::stripScriptingAttributes > > (this=0x95178430130, attributeVector=...) at > > ../../third_party/WebKit/Source/core/dom/Element.cpp:1350 > > #2 0x00007fffef48bb83 in blink::setAttributes (element=0x95178430130, > > token=0x7fffcbc38680, > > parserContentPolicy=blink::DisallowScriptingAndPluginContent) at > > ../../third_party/WebKit/Source/core/html/parser/HTMLConstructionSite.cpp:65 > > #3 0x00007fffef48cf1d in blink::HTMLConstructionSite::createHTMLElement > > (this=0x2547b7498c28, token=0x7fffcbc38680) at > > ../../third_party/WebKit/Source/core/html/parser/HTMLConstructionSite.cpp:757 > > #4 0x00007fffef48d42a in > > blink::HTMLConstructionSite::insertSelfClosingHTMLElement > (this=0x2547b7498c28, > > token=0x7fffcbc38680) at > > ../../third_party/WebKit/Source/core/html/parser/HTMLConstructionSite.cpp:633 > > #5 0x00007fffef4d46cb in blink::HTMLTreeBuilder::processStartTagForInBody > > (this=0x2547b7498c10, token=0x7fffcbc38680) at > > ../../third_party/WebKit/Source/core/html/parser/HTMLTreeBuilder.cpp:805 > > #6 0x00007fffef4ce8fb in blink::HTMLTreeBuilder::processStartTag > > (this=0x2547b7498c10, token=0x7fffcbc38680) at > > ../../third_party/WebKit/Source/core/html/parser/HTMLTreeBuilder.cpp:1154 > > > > > > A few more questions about this specific change: > > > > * We could just get rid of disableFrameset and assign the expression > > directly > > > to > > > > m_framesetOk, right? > > Yes, but I prefer the current code as it matches the order described in > https://html.spec.whatwg.org/#parsing-main-inbody algorithm. Yeah, current code is better. It's just that I wanted to make sure that it presents no functional change. Just to make sure, the test you added would have passed regardless of the code change, right?
On 2015/10/02 08:57:42, Yoav Weiss wrote: > On 2015/10/02 08:52:37, kouhei (catching-up) wrote: > > Added tests. Please take a look. > > > > On 2015/10/01 12:38:31, kouhei (catching-up) wrote: > > > OK. I have better understanding of this bug now. Please review code changes > in > > > PS#4. I'll try to add a repro test tomorrow. > > > > > > > On 2015/09/29 07:12:42, Yoav Weiss wrote: > > > > > Can you detail what you suspect is happening in the minimized test case? > > > What > > > > > makes you think this change will give us a better picture of what's > going > > > on? > > > m_tree.insertSelfClosingHTMLElement(token) may end up mutating m_attributes > > > vector, and typeAttribute ptr captured before the call becomes stale. > > > > > > #0 WTF::Vector<blink::Attribute, 0ul, WTF::DefaultAllocator>::shrink > > > (this=0x7fffcbc386a8, size=1) at > > > ../../third_party/WebKit/Source/wtf/Vector.h:1019 > > > #1 0x00007fffef2689ec in blink::Element::stripScriptingAttributes > > > (this=0x95178430130, attributeVector=...) at > > > ../../third_party/WebKit/Source/core/dom/Element.cpp:1350 > > > #2 0x00007fffef48bb83 in blink::setAttributes (element=0x95178430130, > > > token=0x7fffcbc38680, > > > parserContentPolicy=blink::DisallowScriptingAndPluginContent) at > > > ../../third_party/WebKit/Source/core/html/parser/HTMLConstructionSite.cpp:65 > > > #3 0x00007fffef48cf1d in blink::HTMLConstructionSite::createHTMLElement > > > (this=0x2547b7498c28, token=0x7fffcbc38680) at > > > > ../../third_party/WebKit/Source/core/html/parser/HTMLConstructionSite.cpp:757 > > > #4 0x00007fffef48d42a in > > > blink::HTMLConstructionSite::insertSelfClosingHTMLElement > > (this=0x2547b7498c28, > > > token=0x7fffcbc38680) at > > > > ../../third_party/WebKit/Source/core/html/parser/HTMLConstructionSite.cpp:633 > > > #5 0x00007fffef4d46cb in blink::HTMLTreeBuilder::processStartTagForInBody > > > (this=0x2547b7498c10, token=0x7fffcbc38680) at > > > ../../third_party/WebKit/Source/core/html/parser/HTMLTreeBuilder.cpp:805 > > > #6 0x00007fffef4ce8fb in blink::HTMLTreeBuilder::processStartTag > > > (this=0x2547b7498c10, token=0x7fffcbc38680) at > > > ../../third_party/WebKit/Source/core/html/parser/HTMLTreeBuilder.cpp:1154 > > > > > > > > A few more questions about this specific change: > > > > > * We could just get rid of disableFrameset and assign the expression > > > directly > > > > to > > > > > m_framesetOk, right? > > > > Yes, but I prefer the current code as it matches the order described in > > https://html.spec.whatwg.org/#parsing-main-inbody algorithm. > > Yeah, current code is better. It's just that I wanted to make sure that it > presents no functional change. OK > Just to make sure, the test you added would have passed regardless of the code > change, right? The added test third_party/WebKit/LayoutTests/fast/parser/strip-script-attrs-on-input.html - crashes on ASAN build before CL - passes on ASAN build after CL
On 2015/10/02 09:00:31, kouhei (catching-up) wrote: > On 2015/10/02 08:57:42, Yoav Weiss wrote: > > On 2015/10/02 08:52:37, kouhei (catching-up) wrote: > > > Added tests. Please take a look. > > > > > > On 2015/10/01 12:38:31, kouhei (catching-up) wrote: > > > > OK. I have better understanding of this bug now. Please review code > changes > > in > > > > PS#4. I'll try to add a repro test tomorrow. > > > > > > > > > On 2015/09/29 07:12:42, Yoav Weiss wrote: > > > > > > Can you detail what you suspect is happening in the minimized test > case? > > > > What > > > > > > makes you think this change will give us a better picture of what's > > going > > > > on? > > > > m_tree.insertSelfClosingHTMLElement(token) may end up mutating > m_attributes > > > > vector, and typeAttribute ptr captured before the call becomes stale. > > > > > > > > #0 WTF::Vector<blink::Attribute, 0ul, WTF::DefaultAllocator>::shrink > > > > (this=0x7fffcbc386a8, size=1) at > > > > ../../third_party/WebKit/Source/wtf/Vector.h:1019 > > > > #1 0x00007fffef2689ec in blink::Element::stripScriptingAttributes > > > > (this=0x95178430130, attributeVector=...) at > > > > ../../third_party/WebKit/Source/core/dom/Element.cpp:1350 > > > > #2 0x00007fffef48bb83 in blink::setAttributes (element=0x95178430130, > > > > token=0x7fffcbc38680, > > > > parserContentPolicy=blink::DisallowScriptingAndPluginContent) at > > > > > ../../third_party/WebKit/Source/core/html/parser/HTMLConstructionSite.cpp:65 > > > > #3 0x00007fffef48cf1d in blink::HTMLConstructionSite::createHTMLElement > > > > (this=0x2547b7498c28, token=0x7fffcbc38680) at > > > > > > ../../third_party/WebKit/Source/core/html/parser/HTMLConstructionSite.cpp:757 > > > > #4 0x00007fffef48d42a in > > > > blink::HTMLConstructionSite::insertSelfClosingHTMLElement > > > (this=0x2547b7498c28, > > > > token=0x7fffcbc38680) at > > > > > > ../../third_party/WebKit/Source/core/html/parser/HTMLConstructionSite.cpp:633 > > > > #5 0x00007fffef4d46cb in blink::HTMLTreeBuilder::processStartTagForInBody > > > > (this=0x2547b7498c10, token=0x7fffcbc38680) at > > > > ../../third_party/WebKit/Source/core/html/parser/HTMLTreeBuilder.cpp:805 > > > > #6 0x00007fffef4ce8fb in blink::HTMLTreeBuilder::processStartTag > > > > (this=0x2547b7498c10, token=0x7fffcbc38680) at > > > > ../../third_party/WebKit/Source/core/html/parser/HTMLTreeBuilder.cpp:1154 > > > > > > > > > > A few more questions about this specific change: > > > > > > * We could just get rid of disableFrameset and assign the expression > > > > directly > > > > > to > > > > > > m_framesetOk, right? > > > > > > Yes, but I prefer the current code as it matches the order described in > > > https://html.spec.whatwg.org/#parsing-main-inbody algorithm. > > > > Yeah, current code is better. It's just that I wanted to make sure that it > > presents no functional change. > OK > > > Just to make sure, the test you added would have passed regardless of the code > > change, right? > The added test > third_party/WebKit/LayoutTests/fast/parser/strip-script-attrs-on-input.html > - crashes on ASAN build before CL > - passes on ASAN build after CL Ah, sorry. If you meant the bool condition bug, the behaviour is caught by existing tests. PS#1 (which has the boolean bug you pointed out) failed on the bots: - https://storage.googleapis.com/chromium-layout-test-archives/linux_chromium_r...
On 2015/10/02 09:00:31, kouhei (catching-up) wrote: > On 2015/10/02 08:57:42, Yoav Weiss wrote: > > On 2015/10/02 08:52:37, kouhei (catching-up) wrote: > > > Added tests. Please take a look. > > > > > > On 2015/10/01 12:38:31, kouhei (catching-up) wrote: > > > > OK. I have better understanding of this bug now. Please review code > changes > > in > > > > PS#4. I'll try to add a repro test tomorrow. > > > > > > > > > On 2015/09/29 07:12:42, Yoav Weiss wrote: > > > > > > Can you detail what you suspect is happening in the minimized test > case? > > > > What > > > > > > makes you think this change will give us a better picture of what's > > going > > > > on? > > > > m_tree.insertSelfClosingHTMLElement(token) may end up mutating > m_attributes > > > > vector, and typeAttribute ptr captured before the call becomes stale. > > > > > > > > #0 WTF::Vector<blink::Attribute, 0ul, WTF::DefaultAllocator>::shrink > > > > (this=0x7fffcbc386a8, size=1) at > > > > ../../third_party/WebKit/Source/wtf/Vector.h:1019 > > > > #1 0x00007fffef2689ec in blink::Element::stripScriptingAttributes > > > > (this=0x95178430130, attributeVector=...) at > > > > ../../third_party/WebKit/Source/core/dom/Element.cpp:1350 > > > > #2 0x00007fffef48bb83 in blink::setAttributes (element=0x95178430130, > > > > token=0x7fffcbc38680, > > > > parserContentPolicy=blink::DisallowScriptingAndPluginContent) at > > > > > ../../third_party/WebKit/Source/core/html/parser/HTMLConstructionSite.cpp:65 > > > > #3 0x00007fffef48cf1d in blink::HTMLConstructionSite::createHTMLElement > > > > (this=0x2547b7498c28, token=0x7fffcbc38680) at > > > > > > ../../third_party/WebKit/Source/core/html/parser/HTMLConstructionSite.cpp:757 > > > > #4 0x00007fffef48d42a in > > > > blink::HTMLConstructionSite::insertSelfClosingHTMLElement > > > (this=0x2547b7498c28, > > > > token=0x7fffcbc38680) at > > > > > > ../../third_party/WebKit/Source/core/html/parser/HTMLConstructionSite.cpp:633 > > > > #5 0x00007fffef4d46cb in blink::HTMLTreeBuilder::processStartTagForInBody > > > > (this=0x2547b7498c10, token=0x7fffcbc38680) at > > > > ../../third_party/WebKit/Source/core/html/parser/HTMLTreeBuilder.cpp:805 > > > > #6 0x00007fffef4ce8fb in blink::HTMLTreeBuilder::processStartTag > > > > (this=0x2547b7498c10, token=0x7fffcbc38680) at > > > > ../../third_party/WebKit/Source/core/html/parser/HTMLTreeBuilder.cpp:1154 > > > > > > > > > > A few more questions about this specific change: > > > > > > * We could just get rid of disableFrameset and assign the expression > > > > directly > > > > > to > > > > > > m_framesetOk, right? > > > > > > Yes, but I prefer the current code as it matches the order described in > > > https://html.spec.whatwg.org/#parsing-main-inbody algorithm. > > > > Yeah, current code is better. It's just that I wanted to make sure that it > > presents no functional change. > OK > > > Just to make sure, the test you added would have passed regardless of the code > > change, right? > The added test > third_party/WebKit/LayoutTests/fast/parser/strip-script-attrs-on-input.html > - crashes on ASAN build before CL > - passes on ASAN build after CL Oh, OK. What's the reason for that? Do m_tree.reconstructTheActiveFormattingElements() or m_tree.insertSelfClosingHTMLElement(token) have side-effects that somehow impact typeAttribute?
On 2015/10/02 09:03:49, Yoav Weiss wrote: > On 2015/10/02 09:00:31, kouhei (catching-up) wrote: > > On 2015/10/02 08:57:42, Yoav Weiss wrote: > > > On 2015/10/02 08:52:37, kouhei (catching-up) wrote: > > > > Added tests. Please take a look. > > > > > > > > On 2015/10/01 12:38:31, kouhei (catching-up) wrote: > > > > > OK. I have better understanding of this bug now. Please review code > > changes > > > in > > > > > PS#4. I'll try to add a repro test tomorrow. > > > > > > > > > > > On 2015/09/29 07:12:42, Yoav Weiss wrote: > > > > > > > Can you detail what you suspect is happening in the minimized test > > case? > > > > > What > > > > > > > makes you think this change will give us a better picture of what's > > > going > > > > > on? > > > > > m_tree.insertSelfClosingHTMLElement(token) may end up mutating > > m_attributes > > > > > vector, and typeAttribute ptr captured before the call becomes stale. > > > > > > > > > > #0 WTF::Vector<blink::Attribute, 0ul, WTF::DefaultAllocator>::shrink > > > > > (this=0x7fffcbc386a8, size=1) at > > > > > ../../third_party/WebKit/Source/wtf/Vector.h:1019 > > > > > #1 0x00007fffef2689ec in blink::Element::stripScriptingAttributes > > > > > (this=0x95178430130, attributeVector=...) at > > > > > ../../third_party/WebKit/Source/core/dom/Element.cpp:1350 > > > > > #2 0x00007fffef48bb83 in blink::setAttributes (element=0x95178430130, > > > > > token=0x7fffcbc38680, > > > > > parserContentPolicy=blink::DisallowScriptingAndPluginContent) at > > > > > > > ../../third_party/WebKit/Source/core/html/parser/HTMLConstructionSite.cpp:65 > > > > > #3 0x00007fffef48cf1d in blink::HTMLConstructionSite::createHTMLElement > > > > > (this=0x2547b7498c28, token=0x7fffcbc38680) at > > > > > > > > > ../../third_party/WebKit/Source/core/html/parser/HTMLConstructionSite.cpp:757 > > > > > #4 0x00007fffef48d42a in > > > > > blink::HTMLConstructionSite::insertSelfClosingHTMLElement > > > > (this=0x2547b7498c28, > > > > > token=0x7fffcbc38680) at > > > > > > > > > ../../third_party/WebKit/Source/core/html/parser/HTMLConstructionSite.cpp:633 > > > > > #5 0x00007fffef4d46cb in > blink::HTMLTreeBuilder::processStartTagForInBody > > > > > (this=0x2547b7498c10, token=0x7fffcbc38680) at > > > > > ../../third_party/WebKit/Source/core/html/parser/HTMLTreeBuilder.cpp:805 > > > > > #6 0x00007fffef4ce8fb in blink::HTMLTreeBuilder::processStartTag > > > > > (this=0x2547b7498c10, token=0x7fffcbc38680) at > > > > > > ../../third_party/WebKit/Source/core/html/parser/HTMLTreeBuilder.cpp:1154 > > > > > > > > > > > > A few more questions about this specific change: > > > > > > > * We could just get rid of disableFrameset and assign the expression > > > > > directly > > > > > > to > > > > > > > m_framesetOk, right? > > > > > > > > Yes, but I prefer the current code as it matches the order described in > > > > https://html.spec.whatwg.org/#parsing-main-inbody algorithm. > > > > > > Yeah, current code is better. It's just that I wanted to make sure that it > > > presents no functional change. > > OK > > > > > Just to make sure, the test you added would have passed regardless of the > code > > > change, right? > > The added test > > third_party/WebKit/LayoutTests/fast/parser/strip-script-attrs-on-input.html > > - crashes on ASAN build before CL > > - passes on ASAN build after CL > > Oh, OK. > > What's the reason for that? Do m_tree.reconstructTheActiveFormattingElements() > or m_tree.insertSelfClosingHTMLElement(token) have side-effects that somehow > impact typeAttribute? After a discussion on irc, the reason this fixes things is that insertSelfClosingHTMLElement is destroying the token, and we were looking into it afterwards. LGTM % adding a comment to that effect.
Thanks for the review! > > > > Just to make sure, the test you added would have passed regardless of the > > code > > > > change, right? > > > The added test > > > third_party/WebKit/LayoutTests/fast/parser/strip-script-attrs-on-input.html > > > - crashes on ASAN build before CL > > > - passes on ASAN build after CL > > > > Oh, OK. > > > > What's the reason for that? Do m_tree.reconstructTheActiveFormattingElements() > > or m_tree.insertSelfClosingHTMLElement(token) have side-effects that somehow > > impact typeAttribute? > > After a discussion on irc, the reason this fixes things is that > insertSelfClosingHTMLElement is destroying the token, and we were looking into > it afterwards. > > LGTM % adding a comment to that effect. Added a TODO comment to make it obvious. I'll do the rename in separate CL after the weekend.
The CQ bit was checked by kouhei@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1372343002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1372343002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)
The CQ bit was checked by yoav@yoav.ws
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1372343002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1372343002/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/944deb3adba8aae2a1fc793937a93a051f08e7d7 Cr-Commit-Position: refs/heads/master@{#352035} |