The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c415acc1e0cd8ec75e43bcb596f7bd76321f72f8

commit c415acc1e0cd8ec75e43bcb596f7bd76321f72f8
Author: Kouhei Ueno <kouhei@chromium.org>
Date: Mon Aug 06 06:56:03 2018

Prevent promise reject to be sync scheduled during DocumentLoader detach

Bug:  868592 
Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_slimming_paint_v2;master.tryserver.blink:linux_trusty_blink_rel
Change-Id: I5cff4653a62c357e8eb9d5a82a11b8018653b712
Reviewed-on: https://chromium-review.googlesource.com/1163235
Reviewed-by: Yuki Shiino <yukishiino@chromium.org>
Reviewed-by: Yutaka Hirano <yhirano@chromium.org>
Commit-Queue: Kouhei Ueno <kouhei@chromium.org>
Cr-Commit-Position: refs/heads/master@{#580814}
[modify] https://crrev.com/c415acc1e0cd8ec75e43bcb596f7bd76321f72f8/third_party/WebKit/LayoutTests/FlagExpectations/enable-blink-gen-property-trees
[delete] https://crrev.com/885bca11d90d1f45551004bbc5bb0ba99df7d291/third_party/WebKit/LayoutTests/performance/performance-observer-crash-expected.txt
[delete] https://crrev.com/885bca11d90d1f45551004bbc5bb0ba99df7d291/third_party/WebKit/LayoutTests/performance/performance-observer-crash.html
[modify] https://crrev.com/c415acc1e0cd8ec75e43bcb596f7bd76321f72f8/third_party/blink/renderer/core/fetch/fetch_manager.cc

> c#36
Thanks for giving this a thought.

I'm worried that "will be destroyed soon" further complicates DocumentLifecycle, the is_context_destroyed_ flag is already outside that, which is already bad.

As a counter proposal, until tzik@'s document->microtaskqueue attribution is done, would it make sense to forbid non-posttask promise resolve during FrameLoader::PrepareForCommit ?
While this is also a hack, I think the intent of FrameLoader::PrepareForCommit is to invoke all possible js exec before document shutdown.

Actually, the flag will end-up in ExecutionContext anyways, so c#38 is essentially no difference from c#6

I'm going to create patch for c#36==c#38

Discussed offline, and will proceed with additional checkpoitn
https://chromium-review.googlesource.com/c/chromium/src/+/1164148

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/38894f66a9f1d3142746572da766de1b02f8e2ce

commit 38894f66a9f1d3142746572da766de1b02f8e2ce
Author: Kouhei Ueno <kouhei@chromium.org>
Date: Tue Aug 07 03:45:25 2018

Flush microtask queue before commit

Bug:  868592 
Change-Id: Ia1d17f0b1d07d27a1665d6871545f051ee2eed87
Reviewed-on: https://chromium-review.googlesource.com/1164148
Reviewed-by: Yutaka Hirano <yhirano@chromium.org>
Commit-Queue: Kouhei Ueno <kouhei@chromium.org>
Cr-Commit-Position: refs/heads/master@{#581124}
[modify] https://crrev.com/38894f66a9f1d3142746572da766de1b02f8e2ce/third_party/blink/renderer/core/loader/frame_loader.cc

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a604d09eb9e3a304d0ce61ee18e8596a62b8e9cc

commit a604d09eb9e3a304d0ce61ee18e8596a62b8e9cc
Author: Kouhei Ueno <kouhei@chromium.org>
Date: Tue Aug 07 05:52:49 2018

Revert: Prevent promise reject to be sync scheduled during DocumentLoader detach

Bug:  868592 
Change-Id: Ida5a487e26634b5f424e82a0323acfab939ffe84
Reviewed-on: https://chromium-review.googlesource.com/1164149
Reviewed-by: Yutaka Hirano <yhirano@chromium.org>
Commit-Queue: Kouhei Ueno <kouhei@chromium.org>
Cr-Commit-Position: refs/heads/master@{#581142}
[modify] https://crrev.com/a604d09eb9e3a304d0ce61ee18e8596a62b8e9cc/third_party/blink/renderer/core/fetch/fetch_manager.cc

Thanks, the solution of flushing the microtask queue sounds good. In the future, I think we're going to try to switch Frame on every* navigation, which should hopefully prevent these sorts of issues.

* the exception will probably be instances where we need to reuse the Window object. I'm not sure if it's OK for microtasks to execute on the new Document in this instance?

I'd like to merge the below CL to beta:

https://chromium.googlesource.com/chromium/src.git/+/38894f66a9f1d3142746572da766de1b02f8e2ce

This bug requires manual review: Reverts referenced in bugdroid comments after merge request.
Please contact the milestone owner if you have questions.
Owners: amineer@(Android), kariahda@(iOS), cindyb@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Note that the revert is about original attempt to fix. The merge request is about the second attempt (which seems succeeded)

+ awhalley@ (Security TPM) for M69 merge review.

Please mark security bugs as fixed as soon as the fix lands, and before requesting merges. This update is based on the merge- labels applied to this issue. Please reopen if this update was incorrect.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Let's give it a day longer in canary, but then looks good to merge.

Pls update bug with canary result tomorrow. 

The NextAction date has arrived: 2018-08-10

The CL introduced crash on Canary. https://bugs.chromium.org/p/chromium/issues/detail?id=872320
Fix on CQ: https://chromium-review.googlesource.com/c/chromium/src/+/1170160

Pls update bug with canary result on Monday morning.

 awhalley@, is this important to merge to M69 or can it wait until M70? Checking as first CL introduced crash on Canary per comment #53.

kouhei@ - did any of your investigation on this (e.g. comment 30) make you less worried about exploitability?

My thought is that we should still try and get the fix into 69 to be on the safe side, but waiting for some more canary/dev results sounds good.

> c#57
I agree on the level of exploitability:
- We found at least two possible entrypoints to this bug (fetch cancel, img.decode() cancel)
- So far I couldn't exploit UXSS by myself because of other mitigation applied (Swapping renderer process on nav, OOPIF). All the PoC are same-origin.

I'd still like to have this merged up to stable eventually, but want to wait for more crash rate data

Update: The "flush microtaskqueue" CL uncovered another bug, investigating.

I'm wondering if we should explore another fix (punt the microtask instead of flushing) or continue this route.
I'm very mixed:
- The bugs being uncovered from "flush microtaskqueue" CL are technically not regressions. They were bugs anyway, but either the frequency of hitting the crash increased by CL, or they started producing different stack traces
- The punt fix is likely to be more safe, but will introduce slight difference in semantics (the reject handler no longer runs)

Any thoughts appreciated.

+kinuko

Nevermind my previous comment. It's Restrict-View-SecurityNotify not Restrict-View-SecurityTeam, which looks like all committers are in.

> https://bugs.chromium.org/p/chromium/issues/detail?id=872672#c5

OK. Let's revert https://crrev.com/38894f66a9f1d3142746572da766de1b02f8e2ce to explore c#36

Reg: #59, copy-pasting my comments from the other crash issue here to share opinions/contexts.

----
This concerns me a bit, so let me add a few more people + some context. The CL https://crrev.com/c/1164148 started to flush remaining microtasks right before a new navigation commits, which itself looks right but after that we started to have crashes like this because when those microtasks run we have a frame but may not have a DocumentLoader, while a lot of code used to assume that we have DocumentLoader when we have Frame.  The assumption have been wrong in the previous state as well, but now it happens a lot more often.

My concern is we may have more even if we can just fix this particular one. Options can be: 1) we can keep fixing such crashes, and see if the crash rate becomes okay before branch cut. If not we can revert the original CL (and re-land after branch cut and repeat the same), or 2) revert the original CL and see if we can take another approach.

We're currently in mode 1). This might be just okay but wanted to know if others have other thoughts.

The NextAction date has arrived: 2018-08-13

Holding off c64 actions (revert 38894f66 & land c#36 approach) from discussion with kinuko@

(Settings status to Untriaged again to get attention)

Security team: Would you give us input re: severity of the bug + next actions?
- Since this is a long thread, please refer to c#65, #59, c#58 for our own analysis.
- Worst case scenario is UXSS.
  - So far we weren't able to create UXSS on desktop Chrome since OOPIF + not reusing renderer process is serving as mitigation.
  - We haven't tested this on Android, where OOPIF / renderer process reuse logic is different.
- We don't believe that the issue is widely known externally

alexmos@ - mind taking a look? Interested in if you think Site Isolation is indeed a strong mitigation, the collolory to which being is this an issue on Android.

Seems like site isolation is indeed a strong mitigation for this, as it guarantees that the task can only (incorrectly) run in same-site documents.  However, as of M67 strict site isolation is currently shipped to 99% of all desktop stable users, so remaining 1% would still be subject to this bug.  We were planning to keep that 1% holdback for at least M68 to monitor perf regressions, etc.  We'll discuss how soon we can remove that holdback.

To investigate #68 without site isolation, you can run with --disable-site-isolation-trials.

On Android, we don't yet have any site isolation on stable (outside of an available enterprise policy).  We are currently running trials on canary/dev/beta to figure out a shippable form of site isolation, since so far strict site isolation is proving to be too expensive.  I don't see why this issue wouldn't affect Android as well, though we should try out the repros there to verify.  We do swap processes for browser-initiated main frame navigations even without site isolation on Android, but there'd still be plenty of other cases that might lead to a cross-site leak.

Let me unassign this from me for now (I'll continue to pay close attention to this bug). I've landed fix in ToT while the CL uncovered a lot of existing bugs.

We are waiting for the call whether we should continue the merge path.
 

Is anything need to be merge here? If not, pls remove "Merge-Review-69" label. 

Nothing to merge as we don't have a fix yet. Moving to Chrome 70 and we can track progress there.

We do have a fix landed on ToT: https://chromium.googlesource.com/chromium/src.git/+/38894f66a9f1d3142746572da766de1b02f8e2ce

kouhei@ -- who would be a more appropriate person to drive this issue to fix and merge in Beta/Stable, if approved? Can you please assign the bug to them? Thanks.

I can drive the merges. What I'm not sure is whether if we should do the merge.

We have a fix CL + follow-up CLs landed for M70/ToT which seems to have stabilized for now, but had issues (which lead to follow-up CLs) when we initially landed. 

I want input from the Security team + Release Managers if this bug is severe enough to risk the merge to M69/M68.

Given that we've not got any mitigation on Android, and even on Desktop Site Isolation doesn't fully protect the origin, we should take this for 69.

Change has had 14 days in canary and 7 days in Dev. Hopefully we can get this in time for this week's Beta.

I agree with #79. I know it's a lot of work; I'm sorry. :)

This bug requires manual review: We are only 12 days from stable.
Please contact the milestone owner if you have questions.
Owners: amineer@(Android), kariahda@(iOS), cindyb@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Could you pls point the Cl to merge?

The CLs that should be merged are:
- https://crrev.com/c415acc1e0cd8ec75e43bcb596f7bd76321f72f8 (only the test removals)
- https://chromium.googlesource.com/chromium/src.git/+/38894f66a9f1d3142746572da766de1b02f8e2ce
- https://chromium.googlesource.com/chromium/src.git/+/d995adf5879e71074e33983b0b038d21b3f3be43
- https://chromium.googlesource.com/chromium/src.git/+/a2a107d712d0fb754cc03ccb36630fa3ddc90f14

thanks kouhei@

govind@ - good for 69

Approving merge for all CLs listed at #84 to M69 branch 3497 based on comments #79, #80 and #85. 
Pls merge ASAP so we can take the merges in for this week beta release. Thank you.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/222c9ba7c66822eb0f2e6294443da26ccf9b798e

commit 222c9ba7c66822eb0f2e6294443da26ccf9b798e
Author: Kouhei Ueno <kouhei@chromium.org>
Date: Wed Aug 22 01:31:50 2018

Speculative fix for History::ScrollRestorationInternal null deref

This is a speculative fix for crash reported on crbug.com/872672 .

There is no guarantee that the DocumentLoader is always attached [1],
so let's introduce a null check.

[1] The DocumentLoader may be detached while FrameLoader::PrepareForCommit.

Bug: 872672
Change-Id: I015651506a891c3344f1bdbf40ea013ce988a95f
Reviewed-on: https://chromium-review.googlesource.com/1171972
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Commit-Queue: Kouhei Ueno <kouhei@chromium.org>
Cr-Commit-Position: refs/heads/master@{#582509}
(cherry picked from commit a2a107d712d0fb754cc03ccb36630fa3ddc90f14)

NavigatorServiceWorker: Avoid instantiating if being navigated away.

This CL fixes a clusterfuzz crash which fails to minimize.

Bug:  872320 
Change-Id: Ied4ba2d6143573a4b66fc85fc4fc0fd3b2fbc0ec
Reviewed-on: https://chromium-review.googlesource.com/1170160
Reviewed-by: Hiroki Nakagawa <nhiroki@chromium.org>
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Commit-Queue: Kouhei Ueno <kouhei@chromium.org>
Cr-Commit-Position: refs/heads/master@{#582126}
(cherry picked from commit d995adf5879e71074e33983b0b038d21b3f3be43)

Flush microtask queue before commit

Bug:  868592 
Change-Id: Ia1d17f0b1d07d27a1665d6871545f051ee2eed87
Reviewed-on: https://chromium-review.googlesource.com/1164148
Reviewed-by: Yutaka Hirano <yhirano@chromium.org>
Commit-Queue: Kouhei Ueno <kouhei@chromium.org>
Cr-Commit-Position: refs/heads/master@{#581124}
(cherry picked from commit 38894f66a9f1d3142746572da766de1b02f8e2ce)

Prevent promise reject to be sync scheduled during DocumentLoader detach
(% mod: revert fetch_manager.cc change)

(cherry picked from commit c415acc1e0cd8ec75e43bcb596f7bd76321f72f8)

Bug:  868592 
Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_slimming_paint_v2;master.tryserver.blink:linux_trusty_blink_rel
Change-Id: I5cff4653a62c357e8eb9d5a82a11b8018653b712
Reviewed-on: https://chromium-review.googlesource.com/1163235
Reviewed-by: Yuki Shiino <yukishiino@chromium.org>
Reviewed-by: Yutaka Hirano <yhirano@chromium.org>
Commit-Queue: Kouhei Ueno <kouhei@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#580814}
Reviewed-on: https://chromium-review.googlesource.com/1184122
Cr-Commit-Position: refs/branch-heads/3497@{#760}
Cr-Branched-From: 271eaf50594eb818c9295dc78d364aea18c82ea8-refs/heads/master@{#576753}
[modify] https://crrev.com/222c9ba7c66822eb0f2e6294443da26ccf9b798e/third_party/WebKit/LayoutTests/FlagExpectations/enable-blink-gen-property-trees
[delete] https://crrev.com/b9e41b5b5522039b2d6bce08ae7ed6f375b0c54e/third_party/WebKit/LayoutTests/performance/performance-observer-crash-expected.txt
[delete] https://crrev.com/b9e41b5b5522039b2d6bce08ae7ed6f375b0c54e/third_party/WebKit/LayoutTests/performance/performance-observer-crash.html
[modify] https://crrev.com/222c9ba7c66822eb0f2e6294443da26ccf9b798e/third_party/blink/renderer/core/frame/history.cc
[modify] https://crrev.com/222c9ba7c66822eb0f2e6294443da26ccf9b798e/third_party/blink/renderer/core/loader/frame_loader.cc
[modify] https://crrev.com/222c9ba7c66822eb0f2e6294443da26ccf9b798e/third_party/blink/renderer/modules/service_worker/navigator_service_worker.cc

M69 merges listed here cause stability regression on M69 (Windows and Mac) and M69 is going to stable next week, Pls see https://bugs.chromium.org/p/chromium/issues/detail?id=872672#c15 for more details.

Given the comment from kinuko@ in #c65, it looks like we are open to potentially unbounded number of crashes that we don't know about. It looks like the safest approach for stability of M69 is to revert the patches from comment 84 to bring it back to previous state. It will lead to reintroducing the security bug, however it can be evaluated separately and follow up fixes landed in M70/71.

Reopening bug since reverts coming soon.

Adding Merge-Request-69 to revert 222c9ba7c66822eb0f2e6294443da26ccf9b798e from the M69 branch.

Approving merge for CL listed at #94 to M69 branch 3497 based on comment #92 and internal mail thread/group chat.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/53746ec1f57069de42beeeafb38d1ed2da56e148

commit 53746ec1f57069de42beeeafb38d1ed2da56e148
Author: Nasko Oskov <nasko@chromium.org>
Date: Fri Aug 31 21:27:07 2018

Revert "Speculative fix for History::ScrollRestorationInternal null deref"

This reverts commit 222c9ba7c66822eb0f2e6294443da26ccf9b798e.

Reason for revert: Causing crashes on Mac.

Original change's description:
> Speculative fix for History::ScrollRestorationInternal null deref
> 
> This is a speculative fix for crash reported on crbug.com/872672 .
> 
> There is no guarantee that the DocumentLoader is always attached [1],
> so let's introduce a null check.
> 
> [1] The DocumentLoader may be detached while FrameLoader::PrepareForCommit.
> 
> Bug: 872672
> Change-Id: I015651506a891c3344f1bdbf40ea013ce988a95f
> Reviewed-on: https://chromium-review.googlesource.com/1171972
> Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
> Commit-Queue: Kouhei Ueno <kouhei@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#582509}
> (cherry picked from commit a2a107d712d0fb754cc03ccb36630fa3ddc90f14)
> 
> NavigatorServiceWorker: Avoid instantiating if being navigated away.
> 
> This CL fixes a clusterfuzz crash which fails to minimize.
> 
> Bug:  872320 
> Change-Id: Ied4ba2d6143573a4b66fc85fc4fc0fd3b2fbc0ec
> Reviewed-on: https://chromium-review.googlesource.com/1170160
> Reviewed-by: Hiroki Nakagawa <nhiroki@chromium.org>
> Reviewed-by: Matt Falkenhagen <falken@chromium.org>
> Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
> Commit-Queue: Kouhei Ueno <kouhei@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#582126}
> (cherry picked from commit d995adf5879e71074e33983b0b038d21b3f3be43)
> 
> Flush microtask queue before commit
> 
> Bug:  868592 
> Change-Id: Ia1d17f0b1d07d27a1665d6871545f051ee2eed87
> Reviewed-on: https://chromium-review.googlesource.com/1164148
> Reviewed-by: Yutaka Hirano <yhirano@chromium.org>
> Commit-Queue: Kouhei Ueno <kouhei@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#581124}
> (cherry picked from commit 38894f66a9f1d3142746572da766de1b02f8e2ce)
> 
> Prevent promise reject to be sync scheduled during DocumentLoader detach
> (% mod: revert fetch_manager.cc change)
> 
> (cherry picked from commit c415acc1e0cd8ec75e43bcb596f7bd76321f72f8)
> 
> Bug:  868592 
> Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_slimming_paint_v2;master.tryserver.blink:linux_trusty_blink_rel
> Change-Id: I5cff4653a62c357e8eb9d5a82a11b8018653b712
> Reviewed-on: https://chromium-review.googlesource.com/1163235
> Reviewed-by: Yuki Shiino <yukishiino@chromium.org>
> Reviewed-by: Yutaka Hirano <yhirano@chromium.org>
> Commit-Queue: Kouhei Ueno <kouhei@chromium.org>
> Cr-Original-Commit-Position: refs/heads/master@{#580814}
> Reviewed-on: https://chromium-review.googlesource.com/1184122
> Cr-Commit-Position: refs/branch-heads/3497@{#760}
> Cr-Branched-From: 271eaf50594eb818c9295dc78d364aea18c82ea8-refs/heads/master@{#576753}

TBR=yukishiino@chromium.org,yhirano@chromium.org,haraken@chromium.org,kouhei@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 872672,  872320 ,  868592 
Change-Id: I9f03a1b330dab1ccc68e123212b68bff336bf3ed
Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_slimming_paint_v2;master.tryserver.blink:linux_trusty_blink_rel
Reviewed-on: https://chromium-review.googlesource.com/1200371
Reviewed-by: Nasko Oskov <nasko@chromium.org>
Cr-Commit-Position: refs/branch-heads/3497@{#862}
Cr-Branched-From: 271eaf50594eb818c9295dc78d364aea18c82ea8-refs/heads/master@{#576753}
[modify] https://crrev.com/53746ec1f57069de42beeeafb38d1ed2da56e148/third_party/WebKit/LayoutTests/FlagExpectations/enable-blink-gen-property-trees
[add] https://crrev.com/53746ec1f57069de42beeeafb38d1ed2da56e148/third_party/WebKit/LayoutTests/performance/performance-observer-crash-expected.txt
[add] https://crrev.com/53746ec1f57069de42beeeafb38d1ed2da56e148/third_party/WebKit/LayoutTests/performance/performance-observer-crash.html
[modify] https://crrev.com/53746ec1f57069de42beeeafb38d1ed2da56e148/third_party/blink/renderer/core/frame/history.cc
[modify] https://crrev.com/53746ec1f57069de42beeeafb38d1ed2da56e148/third_party/blink/renderer/core/loader/frame_loader.cc
[modify] https://crrev.com/53746ec1f57069de42beeeafb38d1ed2da56e148/third_party/blink/renderer/modules/service_worker/navigator_service_worker.cc

Note that is ONLY reverted on M69 branch and not on Master. This needs to be either fixed on master or the entire patch reverted on master. Will let kouhei@ handle this when he sees this next week.

Important: M70 has branched too, so whatever fix comes needs to go on M70 as well.

[Stability Sheriff]

- The cherry pick was directly into the M69 stable RC, the stable builders will run tests to catch possible regressions from the revert.

- M70 branched on Thursday, but the first beta won't be for a while.  Getting a stable fix should be a release-block-beta for M70.  Ideally in the next dev release to minimize exposure without the security fix.

Thx

I just posted an update in https://bugs.chromium.org/p/chromium/issues/detail?id=872672#c19, which may indicate which change is needed to get this security fix back into M69.

(The History::StateInternal crash from issue 879477 wasn't resolved in the History::ScrollRestorationInternal fix in issue 872672, and it's the reason for the crashes mentioned in comments 13-15 of issue 872672 which led to this revert.)

Note: We're aiming to get this fix back into M69 ASAP, once the remaining crashes are resovled.  This is most important for Android where there's no Site Isolation (see comments 68 and 71), but we want it on desktop as well (see comment 79).

Adding "RBS" for M69 per comment #102 for future respin. 

+ Chrome on Android and Chrome OS M69 Release TPM, ptal comments #102 and #103. Thank you.

Long thread. I'm trying to summarize the flow that lead to the merge decision. I'll follow-up with what is happening after the merge.

TL;DR: The stability risky merge was done since it is a serious security bug with a possible public exposure and with no Android mitigation.

- c#0 ccordry@ reports that some javascript state in previous page was still present after the navigation. This was reported as a unexpected behavior bug, not security bug.
- c#13 dcheng@ realizes that this bug may be a security bug if it allows code to be executed in the new page. -> The bug is considered public from its exposure from c#0->c#12.
- c#18 hiroshige@ and kouhei@ minimizes the issue to that "{fetch,imgdecode} reject promises are run on the next page". Confirms that the site isolation works as a mitigation on Desktop for UXSS navs.
- c#37 kouhei@ lands the fix A, which force defers running promise reject handlers so that they can be discarded if they attempt to run on the next page. This was reverted in c#43 being too hacky (adding dependency to another hack which is marked to be removed, and being cryptic).
- c#42 kouhei@ lands the fix B, which flushes microtask queue before committing navigation so that they will run on previous page. This opens up a can of worms, since this greatly increased the chance of running js with detached DocumentLoader (which was still possible before the patch, but getting there was tricky enough).
- c#65 kinuko@ expresses concern proceeding with the merge, from the number of follow-up patches that needed to be landed to stabilize Canary.
- c#66-86 We decide to proceed with the merge, since (a) the issue is serious security bug UXSS, (b) the issue is still present on Android, and (c) the issue is considered to be public from its exposure c#0-c#13.

next AIs:
- Confirm the stability issue fix after the canary is released.
- Merge the fix to the M70 branch.
- Redo the merge + the creis@ follow-up fix to M69 respin.

Summary after the merge.
- c#87 kouhei@ merges the fix + its follow-ups to M69 branch.
- c#91 govind@ discovers the stability has regressed after the merge CL.
- c#96 nasko@ reverts the merge in M69. The fix still present on ToT.
- crbug/872672 c#22 creis@ lands follow-up fix on ToT.
- c#101-104 creis@ proposes to retry the merge in M69 stable respin after we confirm the stability issue fixed in the follow-up fix. Waiting for the Release TPMs to confirm.
- crbug/879933 blocks the canary release to confirm the stability issue fix.

Update on windows canary release: crbug/879933 bug is fixed now, 71.0.3541.0 successfully deployed to windows canary.

[stability sheriff] Removing from the queue since it looks like the right people have active attention on this. Add it back if you'd still like sheriff attention.

kouhei@: Thanks for the summary in comments 105-106!  My followup fix in issue 872672 seems to be resolving the remaining blink::History crashes.  I've requested merge to M70.

Note: There's another crash to understand in issue 881126 before we repeat the merge to M69.  (Related to  issue 872320 , which might not have been fixed on the M69 branch.)

dcheng@ and I had a chat how to proceed with the short term fix that is mergeable.

We don't have a good answer yet. The options we have so far seems to be:

Option 1: Continue the current path of adding DocumentLoader nullchk.
[pro] Most secure. Should mitigate similar UXSS attack surface.
[con] Unstable.

Option 2: Move where we flush microtask queue in PrepareForCommit. (e.g. move to in middle of DetachDocumentLoader)
[pro] Equally secure as Option 1.
[con] Unknown stability. This route would ensure having a non-null DocumentLoader* attached to a frame, but the DocumentLoader state is after detach.

Option 3: Retry c#37 type fixes.
[pro] Most stable.
[con] Less secure compared to Option 1&2. While this mitigates the fetch() reject attack, this is still vulnerable to similar attacks. (hiroshige@ was able to find img.decode() repro for example)

Comment 113: Option 1 has the advantage that (most of) it has been baking on M70 already, apart from the upcoming fix for issue 881126.  That seems like it might let us get a fix into M69 sooner than trying option 2, even if that might help us avoid some crashes.  Maybe option 2 is worth trying on trunk, once we get M69 taken care of?  (I don't see much benefit to option 3 if we think it won't be secure.)

How soon can we get the fix for issue 881126?  It looks like we're likely to miss the respin deadline for both Android (today) and Desktop (tomorrow), so we may end up needing a separate respin to get this bug fixed on M69, which is unfortunate.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6be8b5a07bdfa95c37e2da9cace7d7d4b69b31b5

commit 6be8b5a07bdfa95c37e2da9cace7d7d4b69b31b5
Author: Kouhei Ueno <kouhei@chromium.org>
Date: Fri Sep 07 03:07:42 2018

Speculative crash fix for navigator.serviceworker access during unload

This should fix crash/caab6eb137e58385

This CL addresses the unhandled case in crrev.com/582126

TBR=falken@chromium.org

Bug: 881126,  868592 
Change-Id: I906eecc3bf21aa9900355b1f312bd5025375fa8d
Reviewed-on: https://chromium-review.googlesource.com/1207781
Commit-Queue: Kouhei Ueno <kouhei@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Nasko Oskov <nasko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#589419}
[modify] https://crrev.com/6be8b5a07bdfa95c37e2da9cace7d7d4b69b31b5/content/renderer/render_frame_impl.cc
[modify] https://crrev.com/6be8b5a07bdfa95c37e2da9cace7d7d4b69b31b5/third_party/blink/renderer/modules/service_worker/navigator_service_worker.cc

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/20fc7efa908cec0d91b931ae467e1f86619725b0

commit 20fc7efa908cec0d91b931ae467e1f86619725b0
Author: Kouhei Ueno <kouhei@chromium.org>
Date: Fri Sep 07 03:30:08 2018

Speculative crash fix for navigator.serviceworker access during unload

This should fix crash/caab6eb137e58385

This CL addresses the unhandled case in crrev.com/582126

TBR=falken@chromium.org

Bug: 881126,  868592 
Change-Id: I906eecc3bf21aa9900355b1f312bd5025375fa8d
Reviewed-on: https://chromium-review.googlesource.com/1207781
Commit-Queue: Kouhei Ueno <kouhei@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Nasko Oskov <nasko@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#589419}(cherry picked from commit 6be8b5a07bdfa95c37e2da9cace7d7d4b69b31b5)
Reviewed-on: https://chromium-review.googlesource.com/1212368
Reviewed-by: Kouhei Ueno <kouhei@chromium.org>
Cr-Commit-Position: refs/branch-heads/3545@{#2}
Cr-Branched-From: a2bbe9dedf867fccce6d8073dc8e9c864c662bfe-refs/heads/master@{#589377}
[modify] https://crrev.com/20fc7efa908cec0d91b931ae467e1f86619725b0/content/renderer/render_frame_impl.cc
[modify] https://crrev.com/20fc7efa908cec0d91b931ae467e1f86619725b0/third_party/blink/renderer/modules/service_worker/navigator_service_worker.cc

Issue 881126 is now fixed and r589419 will likely be merged to M70 on Monday.

Current plan is to merge everything (i.e., mega-merge from comment 87, plus r588227 from issue 872672, plus r589419 from issue 881126) to M69 at some point *after* Monday's Desktop and Android RC cuts.  That will allow an urgent respin to go out without these fixes while we gain more confidence in these fixes on M70.  That's important since we're hesitant to include something potentially unstable in next week's respin.  The extra bake time on M70 should give us more confidence.  We'll likely do the combined merge after r589419 has been on M70 for a few days, and once we've checked that there aren't other crashes that track back to all of this.

The goal is still to get the combined set of fixes into M69 on both Desktop and Android as soon as they seem safe.

No longer on the Chrome team, e-mail me @google.com if any attention still required from me here, otherwise good luck!

awhalley@ - is there anything needed for M70?

Comment 119: M70 has the security fix plus a large number of stability fixes-- all but r589419 for issue 881126, which I've just requested a M70 merge for on that bug.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e1e00f6a50aaf2ce8975d66a81897220ccbdeba7

commit e1e00f6a50aaf2ce8975d66a81897220ccbdeba7
Author: Kouhei Ueno <kouhei@chromium.org>
Date: Mon Sep 10 20:36:32 2018

[Merge to M70] Speculative crash fix for navigator.serviceworker access during unload

This should fix crash/caab6eb137e58385

This CL addresses the unhandled case in crrev.com/582126

TBR=falken@chromium.org

Bug: 881126,  868592 
Change-Id: I906eecc3bf21aa9900355b1f312bd5025375fa8d
Reviewed-on: https://chromium-review.googlesource.com/1207781
Commit-Queue: Kouhei Ueno <kouhei@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Nasko Oskov <nasko@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#589419}(cherry picked from commit 6be8b5a07bdfa95c37e2da9cace7d7d4b69b31b5)
Reviewed-on: https://chromium-review.googlesource.com/1217165
Reviewed-by: Charlie Reis <creis@chromium.org>
Cr-Commit-Position: refs/branch-heads/3538@{#246}
Cr-Branched-From: 79f7c91a2b2a2932cd447fa6f865cb6662fa8fa6-refs/heads/master@{#587811}
[modify] https://crrev.com/e1e00f6a50aaf2ce8975d66a81897220ccbdeba7/content/renderer/render_frame_impl.cc
[modify] https://crrev.com/e1e00f6a50aaf2ce8975d66a81897220ccbdeba7/third_party/blink/renderer/modules/service_worker/navigator_service_worker.cc

Removing release block beta label.

Thanks for the M70 merge. I'll prepare a mega patch for M69 merge

M69 merge CL: https://chromium-review.googlesource.com/c/chromium/src/+/1218183

To be merged after M70 stats lg.

I've sanity checked that the crashes are gone from M70 for issue 881126 (ServiceWorker) and issue 872672 (History).  I've also looked to see if other crashes may have been caused by this fix when it was present on M69, and I haven't found any others.  Specifically, I looked at the top 300 crash signatures in the comparison links below:

Going from 69.0.3497.42 to 69.0.3497.57, when the fix was first merged to M69, we saw the History and ServiceWorker crashes start:
https://crash.corp.google.com/browse?q=product_name%3D%27Chrome%27&compProp=product.Version&v1=69.0.3497.42&v2=69.0.3497.57#magicsignature:300,-magicsignature2:30,-stablesignature:30

Going from 69.0.3497.72 to 69.0.3497.81, when the fix was reverted from M69, we saw those two crashes go away:
https://crash.corp.google.com/browse?q=product_name%3D%27Chrome%27&compProp=product.Version&v1=69.0.3497.81&v2=69.0.3497.72#magicsignature:300,-magicsignature2:30,-stablesignature:30

No other renderer crashes seemed to appear and disappear in those same versions, at least with any possible connection to this bug.  Skimming the top few, I didn't see other bugs with possible connections either.

I think at this point, we may be safe to merge the combined set of fixes (in kouhei@'s https://chromium-review.googlesource.com/c/chromium/src/+/1218183 mega-merge CL) to M69.  I'll double check with dcheng@ and add a merge request if he agrees.

I've verified the fix applies cleanly and builds on my local M69 branch, and dcheng@ agrees it should be safe to proceed at this point.  Requesting merge to M69 for the updated combined merge at https://chromium-review.googlesource.com/c/chromium/src/+/1218183, which contains the UXSS fix and all the necessary stability fixes.

This bug requires manual review: Request affecting a post-stable build
Please contact the milestone owner if you have questions.
Owners: amineer@(Android), kariahda@(iOS), cindyb@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Approving merge to M69 branch 3497 based on comment #126 and per offline group chat. Pls merge tomorrow morning after beta coverage. Thank you.

The NextAction date has arrived: 2018-09-14

Double checked the crashes after 70.0.3538.16 has been on Beta for a day.  No new crashes in M70 for either issue 881126 or issue 872672.  I'll proceed with the merge!

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f3c120ac4416a678e18ee28294c52cd517c147c9

commit f3c120ac4416a678e18ee28294c52cd517c147c9
Author: Kouhei Ueno <kouhei@chromium.org>
Date: Fri Sep 14 17:42:50 2018

M69 Mega-patch for 868592 fix

This CL is a collection of cherry-picks related to  crbug.com/868592  fix.
Specifically, this is:
- The original mega-patch crrev.com/222c9ba7c6
- creis@ follow-up fix crrev.com/27986c7c955
- kouhei@ follow-up fix crrev.com/6be8b5a07bdf

The original change descriptions are captured below % Change-Id lines

---

Speculative crash fix for navigator.serviceworker access during unload

This should fix crash/caab6eb137e58385

This CL addresses the unhandled case in crrev.com/582126

TBR=falken@chromium.org

Bug: 881126,  868592 
Reviewed-on: https://chromium-review.googlesource.com/1207781
Commit-Queue: Kouhei Ueno <kouhei@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Nasko Oskov <nasko@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#589419}(cherry picked from commit 6be8b5a07bdfa95c37e2da9cace7d7d4b69b31b5)
Reviewed-on: https://chromium-review.googlesource.com/1212368
Reviewed-by: Kouhei Ueno <kouhei@chromium.org>
Cr-Commit-Position: refs/branch-heads/3545@{#2}
Cr-Branched-From: a2bbe9dedf867fccce6d8073dc8e9c864c662bfe-refs/heads/master@{#589377}

Speculative fix for additional History DocumentLoader crashes.

There is no guarantee that the DocumentLoader is always attached [1],
so let's introduce null checks in StateInternal and setScrollRestoration.

[1] The DocumentLoader may be detached while FrameLoader::PrepareForCommit.

BUG=879477, 872672

Reviewed-on: https://chromium-review.googlesource.com/1200075
Commit-Queue: Charlie Reis <creis@chromium.org>
Reviewed-by: Nate Chapin <japhet@chromium.org>
Cr-Commit-Position: refs/heads/master@{#588227}
Speculative fix for History::ScrollRestorationInternal null deref

This is a speculative fix for crash reported on crbug.com/872672 .

There is no guarantee that the DocumentLoader is always attached [1],
so let's introduce a null check.

[1] The DocumentLoader may be detached while FrameLoader::PrepareForCommit.

Bug: 872672
Reviewed-on: https://chromium-review.googlesource.com/1171972
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Commit-Queue: Kouhei Ueno <kouhei@chromium.org>
Cr-Commit-Position: refs/heads/master@{#582509}
(cherry picked from commit a2a107d712d0fb754cc03ccb36630fa3ddc90f14)

NavigatorServiceWorker: Avoid instantiating if being navigated away.

This CL fixes a clusterfuzz crash which fails to minimize.

Bug:  872320 
Reviewed-on: https://chromium-review.googlesource.com/1170160
Reviewed-by: Hiroki Nakagawa <nhiroki@chromium.org>
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Commit-Queue: Kouhei Ueno <kouhei@chromium.org>
Cr-Commit-Position: refs/heads/master@{#582126}
(cherry picked from commit d995adf5879e71074e33983b0b038d21b3f3be43)

Flush microtask queue before commit

Bug:  868592 
Reviewed-on: https://chromium-review.googlesource.com/1164148
Reviewed-by: Yutaka Hirano <yhirano@chromium.org>
Commit-Queue: Kouhei Ueno <kouhei@chromium.org>
Cr-Commit-Position: refs/heads/master@{#581124}
(cherry picked from commit 38894f66a9f1d3142746572da766de1b02f8e2ce)

Prevent promise reject to be sync scheduled during DocumentLoader detach
(% mod: revert fetch_manager.cc change)

(cherry picked from commit c415acc1e0cd8ec75e43bcb596f7bd76321f72f8)

Bug:  868592 
Change-Id: I50029416f0441a9f09c538716684a01cb8af93e1
Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_slimming_paint_v2;master.tryserver.blink:linux_trusty_blink_rel
Reviewed-on: https://chromium-review.googlesource.com/1163235
Reviewed-by: Yuki Shiino <yukishiino@chromium.org>
Reviewed-by: Yutaka Hirano <yhirano@chromium.org>
Commit-Queue: Kouhei Ueno <kouhei@chromium.org>
Cr-Original-Original-Commit-Position: refs/heads/master@{#580814}
Reviewed-on: https://chromium-review.googlesource.com/1184122
Cr-Original-Commit-Position: refs/branch-heads/3497@{#760}
Cr-Original-Branched-From: 271eaf50594eb818c9295dc78d364aea18c82ea8-refs/heads/master@{#576753}
Reviewed-on: https://chromium-review.googlesource.com/1218183
Reviewed-by: Kouhei Ueno <kouhei@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Cr-Commit-Position: refs/branch-heads/3497@{#938}
Cr-Branched-From: 271eaf50594eb818c9295dc78d364aea18c82ea8-refs/heads/master@{#576753}
[modify] https://crrev.com/f3c120ac4416a678e18ee28294c52cd517c147c9/content/renderer/render_frame_impl.cc
[modify] https://crrev.com/f3c120ac4416a678e18ee28294c52cd517c147c9/third_party/WebKit/LayoutTests/FlagExpectations/enable-blink-gen-property-trees
[delete] https://crrev.com/47b99a368e694ae38346630ce4c621622de4b8b0/third_party/WebKit/LayoutTests/performance/performance-observer-crash-expected.txt
[delete] https://crrev.com/47b99a368e694ae38346630ce4c621622de4b8b0/third_party/WebKit/LayoutTests/performance/performance-observer-crash.html
[modify] https://crrev.com/f3c120ac4416a678e18ee28294c52cd517c147c9/third_party/blink/renderer/core/frame/history.cc
[modify] https://crrev.com/f3c120ac4416a678e18ee28294c52cd517c147c9/third_party/blink/renderer/core/loader/frame_loader.cc
[modify] https://crrev.com/f3c120ac4416a678e18ee28294c52cd517c147c9/third_party/blink/renderer/modules/service_worker/navigator_service_worker.cc

That should hopefully take care of it.  Thanks kouhei@!

This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot