Assert that we never insert an frame element that already has a frame

Assert that we never insert an frame element that already has a frame

BUG=628942, 631151
R=dominicc@chromium.org

Committed: https://crrev.com/baf4f1f0cca9c704ff01de23e9360a1deef00cb4
Cr-Commit-Position: refs/heads/master@{#436294}

[jochen (gone - plz use gerrit), 2016-07-27 08:21:01]

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

[commit-bot: I haz the power, 2016-07-27 08:21:17]

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-27 10:02:14]

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

[commit-bot: I haz the power, 2016-07-27 10:02:15]

Dry run: This issue passed the CQ dry run.

[jochen (gone - plz use gerrit), 2016-07-27 14:33:37]

Description was changed from

==========
Assert that we never insert an frame element that already has a frame

BUG=628942
R=dominicc@chromium.org
==========

to

==========
Assert that we never insert an frame element that already has a frame

BUG=628942,631151
R=dominicc@chromium.org
==========

[jochen (gone - plz use gerrit), 2016-07-27 14:33:37]

jochen@chromium.org changed reviewers:
+ dcheng@chromium.org

[dcheng, 2016-07-27 15:05:48]

lgtm

[jochen (gone - plz use gerrit), 2016-07-27 15:10:24]

note that this will make clusterfuzz unhappy (issue 631151)

should we still land?

[dcheng, 2016-07-27 15:11:57]

On 2016/07/27 15:10:24, jochen wrote:
> note that this will make clusterfuzz unhappy (issue 631151)
> 
> should we still land?

Hmm. Let me look at that tomorrow when I'm less tired =)

[jochen (gone - plz use gerrit), 2016-07-29 12:42:35]

any updates?

[dominicc (has gone to gerrit), 2016-10-14 08:31:22]

On 2016/07/29 at 12:42:35, jochen wrote:
> any updates?

To summarize, the question as I understand it is: Do we land this patch now and
crash in the security check this adds in cases like those in Issue 631151? Or do
we wait and try to make javascript: IFRAME URLs asynchronous, cutting off Issue
631151, and *then* land this?

dcheng, what do you think?

My $0.02, it boils down to how many crashes the check causes. If we think it is
few or we're not sure, I think we should go ahead and land it. If this did flap
a lot stability sheriffs will have no trouble narrowing down the cause to this
CL.

[jochen (gone - plz use gerrit), 2016-12-02 13:36:24]

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

[commit-bot: I haz the power, 2016-12-02 13:36:42]

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-12-02 13:39:02]

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

[commit-bot: I haz the power, 2016-12-02 13:39:03]

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

[jochen (gone - plz use gerrit), 2016-12-05 07:56:29]

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

[commit-bot: I haz the power, 2016-12-05 07:56:40]

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-12-05 09:31:57]

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

[commit-bot: I haz the power, 2016-12-05 09:31:58]

Dry run: This issue passed the CQ dry run.

[jochen (gone - plz use gerrit), 2016-12-05 15:17:18]

The CQ bit was checked by jochen@chromium.org

[jochen (gone - plz use gerrit), 2016-12-05 15:17:19]

green bots.. so giving this a try

[jochen (gone - plz use gerrit), 2016-12-05 15:17:19]

The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2190523002/#ps20001
(title: "rebase")

[commit-bot: I haz the power, 2016-12-05 15:17:31]

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-12-05 15:21:17]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1480951038642040,
"parent_rev": "df2df50d8200584fdda85c8834b6ef18677d22ba", "commit_rev":
"3ee4d22d1e6ed56eea621f08bca16865383b93ff"}

[commit-bot: I haz the power, 2016-12-05 15:21:53]

Committed patchset #2 (id:20001)

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

Description was changed from

==========
Assert that we never insert an frame element that already has a frame

BUG=628942,631151
R=dominicc@chromium.org
==========

to

==========
Assert that we never insert an frame element that already has a frame

BUG=628942,631151
R=dominicc@chromium.org

Committed: https://crrev.com/baf4f1f0cca9c704ff01de23e9360a1deef00cb4
Cr-Commit-Position: refs/heads/master@{#436294}
==========

[commit-bot: I haz the power, 2016-12-05 15:23:16]

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

[dcheng, 2016-12-05 18:23:50]

Is anyone actively working on the known crashers for this? marius pointed out a
much simpler repro than the CF-reported case:
https://codereview.chromium.org/2497133002/#msg9

And esprehn@ was against landing due to the very simple snippet required to
trigger this.

[jochen (gone - plz use gerrit), 2016-12-07 10:41:54]

On 2016/12/05 at 18:23:50, dcheng wrote:
> Is anyone actively working on the known crashers for this? marius pointed out
a much simpler repro than the CF-reported case:
https://codereview.chromium.org/2497133002/#msg9
> 
> And esprehn@ was against landing due to the very simple snippet required to
trigger this.

ah, sorry, didn't see the other CL. I dunno, so far, I didn't get complaints
about crashes, so maybe it's not that common? OTOH, it's a serious issue
everything we hit that check, and it got ignored so far, so maybe crashing will
help to prioritize fixing the underlying behavior in the DOM parser?