Does this TODO actually need to be done?  I thought we dispatched unload events in all frames as part of detach, and in a quick test, I see the unload event firing in a subframe that navigates from local to remote.

WebFrame::swap calls FrameLoader::PrepareForCommit that calls the unload events:

https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/loader/FrameLoader.cpp?l=986

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

commit 6e8a61ba16f529f02841fb75a50f15ec45726411
Author: lukasza <lukasza@chromium.org>
Date: Wed Jun 14 23:14:22 2017

Avoid unnecessary direct dispatching of unload events from RFI::OnSwapOut.

There is no need to call DispatchUnloadEvent directly from
RenderFrameImpl::OnSwapOut, because the unload events will
be dispatched even without this call - this will happen via
the following call chain:
- Document::DispatchUnloadEvents
- FrameLoader::DispatchUnloadEvent
- LocalFrame::Detach
- WebFrame::Swap
- RenderFrameImpl::OnSwapOut

This also means that the old TODO from the code doesn't need fixing - we
already dispatch unload events for swapped out subframes.

BUG=733314
TEST=Manualy set main frame's window.onunload and verified it still fires.

Review-Url: https://codereview.chromium.org/2941783002
Cr-Commit-Position: refs/heads/master@{#479538}

[modify] https://crrev.com/6e8a61ba16f529f02841fb75a50f15ec45726411/content/renderer/render_frame_impl.cc

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

commit bfda1c11ee19e7ca8fe2c73d33db7ab9be9f5a36
Author: lukasza <lukasza@chromium.org>
Date: Fri Jun 16 19:55:08 2017

Revert of Dispatch unload event for swapped-out subframes. (patchset #2 id:20001 of https://codereview.chromium.org/2941783002/ )

Reason for revert:
This CL is responsible for  https://crbug.com/733940 

Original issue's description:
> Avoid unnecessary direct dispatching of unload events from RFI::OnSwapOut.
>
> There is no need to call DispatchUnloadEvent directly from
> RenderFrameImpl::OnSwapOut, because the unload events will
> be dispatched even without this call - this will happen via
> the following call chain:
> - Document::DispatchUnloadEvents
> - FrameLoader::DispatchUnloadEvent
> - LocalFrame::Detach
> - WebFrame::Swap
> - RenderFrameImpl::OnSwapOut
>
> This also means that the old TODO from the code doesn't need fixing - we
> already dispatch unload events for swapped out subframes.
>
> BUG=733314
> TEST=Manualy set main frame's window.onunload and verified it still fires.
>
> Review-Url: https://codereview.chromium.org/2941783002
> Cr-Commit-Position: refs/heads/master@{#479538}
> Committed: https://chromium.googlesource.com/chromium/src/+/6e8a61ba16f529f02841fb75a50f15ec45726411

TBR=creis@chromium.org
# Not skipping CQ checks because original CL landed more than 1 days ago.
BUG=733314

Review-Url: https://codereview.chromium.org/2938423002
Cr-Commit-Position: refs/heads/master@{#480133}

[modify] https://crrev.com/bfda1c11ee19e7ca8fe2c73d33db7ab9be9f5a36/content/renderer/render_frame_impl.cc

This issue has been Available for over a year. If it's no longer important or seems unlikely to be fixed, please consider closing it out. If it is important, please re-triage the issue.

Sorry for the inconvenience if the bug really should have been left as Available.

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

There's still some cleanup to do here.  

Re: #3, it does look unnecessary to dispatch the unload from OnSwapOut() by explicitly calling DispatchUnloadEvent(), since unload will also be dispatched as part of a later call to frame_->Swap().  The problem that led to the revert in #4 seems to be that in between these two calls, we also mark the RenderView as swapped-out, and running unload might send IPCs that are subject to swapped-out filtering.  Lukasz seems to have fixed one troublesome IPC in  issue 733940 , so it should be ok to try relanding r479538.  We might also want to move the call to SetSwappedOut(true) down to be after frame_->Swap(), which would've avoided this problem, and which also seems more correct because Swap() can fail, and in that case we don't want to mark the RenderView as swapped out.  I'll assign to Lukasz to follow up on this when he gets back.

As for subframe unload handlers, same-site subframe unloads should be dispatched as part of detaching children during Swap.  I've done some work to support running unload in cross-site subframes in  issue 852204 , and arthursonzogni@ is working on a longer-term solution in issue 609963.

The suggestion to move the SetSwappedOut(true) call further down makes sense to me (as a way to postpone filtering of IPCs + to handle failure to swap).  Unfortunately this leads to the following DCHECK:

[1:1:0726/153220.106603:FATAL:render_widget.cc(660)] Check failed: is_swapped_out_.
#0 0x0000048b41bc base::debug::StackTrace::StackTrace()
#1 0x00000480d40b logging::LogMessage::~LogMessage()
#2 0x0000080f8ddc content::RenderWidget::WasSwappedOut()
#3 0x000007cc40fc content::RenderFrameImpl::OnSwapOut()

when running RenderFrameHostManagerTest.InputMsgToSwappedOutRVHIsIgnored in network_service_content_browsertests

Just FYI for #10: I've removed RenderWidget::WasSwappedOut and slightly changed the flow of things in RFI::OnSwapOut in r580576.  Might be worth checking whether the failure here can still happen.  The fact that we hit it at all though is super-weird: I think it means that something was resetting the widget's is_swapped_out_ state to false, but I don't see anything that could've done that between SetSwappedOut and WasSwappedOut, especially when SetSwappedOut was moved to later.  (I'm assuming SetSwappedOut was still called prior to WasSwappedOut, right?)