Crash fix: Avoid using stale HTMLToken after tree construction

Crash fix: Avoid using stale HTMLToken after tree construction

BUG=526286, 468406
Committed: https://crrev.com/944deb3adba8aae2a1fc793937a93a051f08e7d7
Cr-Commit-Position: refs/heads/master@{#352035}

[kouhei (in TOK), 2015-09-29 03:46:17]

kouhei@chromium.org changed reviewers:
+ kinuko@chromium.org, yoav@yoav.ws

[kouhei (in TOK), 2015-09-29 03:46:18]

I failed to create a reliable repro test case for this. I'd like to rely on
ClusterFuzz for regression catching.

[Yoav Weiss, 2015-09-29 06:06:30]

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? :)

[kouhei (in TOK), 2015-09-29 06:46:21]

The CQ bit was checked by kouhei@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-09-29 06:46:26]

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

[commit-bot: I haz the power, 2015-09-29 07:11:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-29 07:11:42]

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...)

[Yoav Weiss, 2015-09-29 07:12:42]

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 )

[kouhei (in TOK), 2015-09-29 07:32:18]

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.

[kouhei (in TOK), 2015-10-01 12:38:31]

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...

[kouhei (in TOK), 2015-10-02 06:35:30]

The CQ bit was checked by kouhei@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-10-02 06:35:47]

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

[commit-bot: I haz the power, 2015-10-02 07:59:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-10-02 07:59:22]

Dry run: This issue passed the CQ dry run.

[kouhei (in TOK), 2015-10-02 08:52:37]

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.

[Yoav Weiss, 2015-10-02 08:57:42]

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?

[kouhei (in TOK), 2015-10-02 09:00:31]

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

[kouhei (in TOK), 2015-10-02 09:02:29]

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...

[Yoav Weiss, 2015-10-02 09:03:49]

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?

[Yoav Weiss, 2015-10-02 09:28:20]

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.

[kouhei (in TOK), 2015-10-02 09:29:45]

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.

[kouhei (in TOK), 2015-10-02 09:29:57]

The CQ bit was checked by kouhei@chromium.org

[commit-bot: I haz the power, 2015-10-02 09:30:26]

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

[commit-bot: I haz the power, 2015-10-02 13:35:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-10-02 13:35:51]

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...)

[Yoav Weiss, 2015-10-02 13:36:43]

The CQ bit was checked by yoav@yoav.ws

[commit-bot: I haz the power, 2015-10-02 13:37:15]

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

[commit-bot: I haz the power, 2015-10-02 14:57:30]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-10-02 14:58:19]

Patchset 6 (id:??) landed as
https://crrev.com/944deb3adba8aae2a1fc793937a93a051f08e7d7
Cr-Commit-Position: refs/heads/master@{#352035}