Fix crash when encountered unmatched script end-tag token in TextMode

Fix crash when encountered unmatched script end-tag token in TextMode

Before this CL, we assumed that <script> start-tag was seen when we
encounter <script> end-tag token when the parser insertion mode is in
'text' mode.

However, the <script> end-tag may also appear w/o script tag in
"the stack of open elements", like in attached test.

BUG=411226
Committed: https://crrev.com/78c93fef96d79c393aedcd7e05b3f6d29c6835b3
Cr-Commit-Position: refs/heads/master@{#406770}

[kouhei (in TOK), 2016-07-14 07:44:27]

kouhei@chromium.org changed reviewers:
+ dominicc@chromium.org

[kouhei (in TOK), 2016-07-14 07:44:37]

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

[commit-bot: I haz the power, 2016-07-14 07:44:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-14 10:53:27]

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

[commit-bot: I haz the power, 2016-07-14 10:53:28]

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

[dominicc (has gone to gerrit), 2016-07-15 02:24:52]

Can you tell me a bit more about this change? Does this work for script tags
*because* an open script tag will flip the parser into script mode, and so the
relevant open script tag (if any) is going to be top of stack? I'm just
wondering if there's ever a time there would be tags under there and we should
be generating implied end tags or something.

[dominicc (has gone to gerrit), 2016-07-15 02:30:57]

*script mode => text mode

On Fri, Jul 15, 2016 at 11:24 AM, <dominicc@chromium.org> wrote:

> Can you tell me a bit more about this change? Does this work for script
> tags
> *because* an open script tag will flip the parser into script mode, and so
> the
> relevant open script tag (if any) is going to be top of stack? I'm just
> wondering if there's ever a time there would be tags under there and we
> should
> be generating implied end tags or something.
>
> https://codereview.chromium.org/2151783002/
>

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[dominicc (has gone to gerrit), 2016-07-15 02:30:57]

*script mode => text mode

On Fri, Jul 15, 2016 at 11:24 AM, <dominicc@chromium.org> wrote:

> Can you tell me a bit more about this change? Does this work for script
> tags
> *because* an open script tag will flip the parser into script mode, and so
> the
> relevant open script tag (if any) is going to be top of stack? I'm just
> wondering if there's ever a time there would be tags under there and we
> should
> be generating implied end tags or something.
>
> https://codereview.chromium.org/2151783002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[kouhei (in TOK), 2016-07-15 07:20:14]

On 2016/07/15 02:30:57, dominicc wrote:
> *script mode => text mode
> 
> On Fri, Jul 15, 2016 at 11:24 AM, <mailto:dominicc@chromium.org> wrote:
> 
> > Can you tell me a bit more about this change? Does this work for script
> > tags
> > *because* an open script tag will flip the parser into script mode, and so
> > the
> > relevant open script tag (if any) is going to be top of stack? I'm just
> > wondering if there's ever a time there would be tags under there and we
> > should
> > be generating implied end tags or something.
> >
> > https://codereview.chromium.org/2151783002/
> >

Ah, I think that was the original intention of the ASSERT.
However, weird case can happen with foreignContent as in attached test.

[dominicc (has gone to gerrit), 2016-07-15 07:40:20]

On 2016/07/15 at 07:20:14, kouhei wrote:
> On 2016/07/15 02:30:57, dominicc wrote:
> > *script mode => text mode
> > 
> > On Fri, Jul 15, 2016 at 11:24 AM, <mailto:dominicc@chromium.org> wrote:
> > 
> > > Can you tell me a bit more about this change? Does this work for script
> > > tags
> > > *because* an open script tag will flip the parser into script mode, and so
> > > the
> > > relevant open script tag (if any) is going to be top of stack? I'm just
> > > wondering if there's ever a time there would be tags under there and we
> > > should
> > > be generating implied end tags or something.
> > >
> > > https://codereview.chromium.org/2151783002/
> > >
> 
> Ah, I think that was the original intention of the ASSERT.
> However, weird case can happen with foreignContent as in attached test.

Yes, no argument here that the ASSERT is wrong. This LGTM; is it possible to
simplify the test at all? Why do we need <y</> and two titles?

[kouhei (in TOK), 2016-07-20 22:40:35]

On 2016/07/15 07:40:20, dominicc wrote:
> On 2016/07/15 at 07:20:14, kouhei wrote:
> > On 2016/07/15 02:30:57, dominicc wrote:
> > > *script mode => text mode
> > > 
> > > On Fri, Jul 15, 2016 at 11:24 AM, <mailto:dominicc@chromium.org> wrote:
> > > 
> > > > Can you tell me a bit more about this change? Does this work for script
> > > > tags
> > > > *because* an open script tag will flip the parser into script mode, and
so
> > > > the
> > > > relevant open script tag (if any) is going to be top of stack? I'm just
> > > > wondering if there's ever a time there would be tags under there and we
> > > > should
> > > > be generating implied end tags or something.
> > > >
> > > > https://codereview.chromium.org/2151783002/
> > > >
> > 
> > Ah, I think that was the original intention of the ASSERT.
> > However, weird case can happen with foreignContent as in attached test.
> 
> Yes, no argument here that the ASSERT is wrong. This LGTM; is it possible to
> simplify the test at all? Why do we need <y</> and two titles?

Changed the y tag to foobar tag.
We need two <title>s to confuse HTMLTreeBuilder simulator.

[dominicc (has gone to gerrit), 2016-07-21 02:02:13]

On 2016/07/20 at 22:40:35, kouhei wrote:
> On 2016/07/15 07:40:20, dominicc wrote:
> > On 2016/07/15 at 07:20:14, kouhei wrote:
> > > On 2016/07/15 02:30:57, dominicc wrote:
> > > > *script mode => text mode
> > > > 
> > > > On Fri, Jul 15, 2016 at 11:24 AM, <mailto:dominicc@chromium.org> wrote:
> > > > 
> > > > > Can you tell me a bit more about this change? Does this work for
script
> > > > > tags
> > > > > *because* an open script tag will flip the parser into script mode,
and so
> > > > > the
> > > > > relevant open script tag (if any) is going to be top of stack? I'm
just
> > > > > wondering if there's ever a time there would be tags under there and
we
> > > > > should
> > > > > be generating implied end tags or something.
> > > > >
> > > > > https://codereview.chromium.org/2151783002/
> > > > >
> > > 
> > > Ah, I think that was the original intention of the ASSERT.
> > > However, weird case can happen with foreignContent as in attached test.
> > 
> > Yes, no argument here that the ASSERT is wrong. This LGTM; is it possible to
> > simplify the test at all? Why do we need <y</> and two titles?
> 
> Changed the y tag to foobar tag.
> We need two <title>s to confuse HTMLTreeBuilder simulator.

I think the reason for the two <title> is is the first <title> is a HTML
integration point, and then the second <title> will flip the mode like HTML
title.

Still LGTM!

[kouhei (in TOK), 2016-07-21 02:11:25]

The CQ bit was checked by kouhei@chromium.org

[commit-bot: I haz the power, 2016-07-21 02:11:43]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-21 02:54:17]

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

[commit-bot: I haz the power, 2016-07-21 02:54:18]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[kouhei (in TOK), 2016-07-21 04:34:49]

The CQ bit was checked by kouhei@chromium.org

[commit-bot: I haz the power, 2016-07-21 04:35:01]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-21 05:13:18]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-07-21 05:15:23]

Description was changed from

==========
Fix crash when encountered unmatched script end-tag token in TextMode

Before this CL, we assumed that <script> start-tag was seen when we
encounter <script> end-tag token when the parser insertion mode is in
'text' mode.

However, the <script> end-tag may also appear w/o script tag in
"the stack of open elements", like in attached test.

BUG=411226
==========

to

==========
Fix crash when encountered unmatched script end-tag token in TextMode

Before this CL, we assumed that <script> start-tag was seen when we
encounter <script> end-tag token when the parser insertion mode is in
'text' mode.

However, the <script> end-tag may also appear w/o script tag in
"the stack of open elements", like in attached test.

BUG=411226

Committed: https://crrev.com/78c93fef96d79c393aedcd7e05b3f6d29c6835b3
Cr-Commit-Position: refs/heads/master@{#406770}
==========

[commit-bot: I haz the power, 2016-07-21 05:15:23]

Patchset 2 (id:??) landed as
https://crrev.com/78c93fef96d79c393aedcd7e05b3f6d29c6835b3
Cr-Commit-Position: refs/heads/master@{#406770}