Disallow script execution after unload event dispatch in frame detach.

Disallow script execution in Document::detach().

There's no reason to run scripts here, since unload events have already
run. Allowing scripts here can lead to re-entrant frame detach, which
results in lots of weird edge cases that Blink wouldn't have to handle
otherwise.

BUG=363099, 371084
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=184035

[dcheng, 2014-10-16 00:56:30]

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

[haraken, 2014-10-16 00:59:27]

LGTM

https://codereview.chromium.org/657263002/diff/1/Source/core/frame/Frame.cpp
File Source/core/frame/Frame.cpp (left):

https://codereview.chromium.org/657263002/diff/1/Source/core/frame/Frame.cpp#...
Source/core/frame/Frame.cpp:111: return;

Shall we add ASSERT(client())?

[dcheng, 2014-10-16 01:21:33]

https://codereview.chromium.org/657263002/diff/1/Source/core/frame/Frame.cpp
File Source/core/frame/Frame.cpp (left):

https://codereview.chromium.org/657263002/diff/1/Source/core/frame/Frame.cpp#...
Source/core/frame/Frame.cpp:111: return;
On 2014/10/16 00:59:27, haraken wrote:
> 
> Shall we add ASSERT(client())?

Done.

https://codereview.chromium.org/657263002/diff/1/Source/core/frame/RemoteFram...
File Source/core/frame/RemoteFrame.cpp (right):

https://codereview.chromium.org/657263002/diff/1/Source/core/frame/RemoteFram...
Source/core/frame/RemoteFrame.cpp:43: if (!client())
I'm not sure if this condition is ever true at the moment, but I don't see a
reason why this might not happen in the future (the same thing that can cause
Blink to try to detach the same LocalFrame multiple times should also be able to
do the same for RemoteFrames).

[dcheng, 2014-10-16 01:21:38]

The CQ bit was checked by dcheng@chromium.org

[commit-bot: I haz the power, 2014-10-16 01:22:33]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/657263002/20001

[dcheng, 2014-10-16 01:40:51]

The CQ bit was unchecked by dcheng@chromium.org

[dcheng, 2014-10-16 01:41:26]

I forgot that extensions can run script at this point. Ugh.

I'm going to see if I can move the call to clearForClose() out of the script
forbidden scope.

[dcheng, 2014-10-16 03:33:14]

PTAL. I've changed this to only suppress scripts in Document::detach, which was
the problematic stack from the bug reports anyway.

[haraken, 2014-10-16 03:52:50]

Still LGTM

[dcheng, 2014-10-16 03:54:02]

The CQ bit was checked by dcheng@chromium.org

[commit-bot: I haz the power, 2014-10-16 03:54:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/657263002/60001

[commit-bot: I haz the power, 2014-10-16 05:28:26]

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

[commit-bot: I haz the power, 2014-10-16 05:28:27]

Try jobs failed on following builders:
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/27220)

[dcheng, 2014-10-20 17:25:34]

The CQ bit was checked by dcheng@chromium.org

[commit-bot: I haz the power, 2014-10-20 17:26:33]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/657263002/60001

[commit-bot: I haz the power, 2014-10-20 17:26:48]

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

[commit-bot: I haz the power, 2014-10-20 17:26:49]

Failed to apply patch for Source/core/frame/Frame.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/core/frame/Frame.cpp
  Hunk #1 FAILED at 103.
  1 out of 1 hunk FAILED -- saving rejects to file
Source/core/frame/Frame.cpp.rej

Patch:       Source/core/frame/Frame.cpp
Index: Source/core/frame/Frame.cpp
diff --git a/Source/core/frame/Frame.cpp b/Source/core/frame/Frame.cpp
index
d45db355c975bbb0c7041fed3c7f2504b8a9c281..0d2331957f5ffcc16808a7110394859ab03dc1e4
100644
--- a/Source/core/frame/Frame.cpp
+++ b/Source/core/frame/Frame.cpp
@@ -103,12 +103,7 @@ void Frame::trace(Visitor* visitor)
 
 void Frame::detach()
 {
-    // client() should never be null because that means we somehow re-entered
-    // the frame detach code... but it is sometimes.
-    // FIXME: Understand why this is happening so we can document this
insanity.
-    // http://crbug.com/371084 is a probable explanation.
-    if (!client())
-        return;
+    ASSERT(m_client);
     // After this, we must no longer talk to the client since this clears
     // its owning reference back to our owning LocalFrame.
     m_client->detached();

[dcheng, 2014-10-21 00:03:29]

The CQ bit was checked by dcheng@chromium.org

[commit-bot: I haz the power, 2014-10-21 00:04:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/657263002/80001

[commit-bot: I haz the power, 2014-10-21 02:05:18]

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

[commit-bot: I haz the power, 2014-10-21 02:05:19]

Exceeded time limit waiting for builds to trigger.

[dcheng, 2014-10-21 02:34:02]

The CQ bit was checked by dcheng@chromium.org

[commit-bot: I haz the power, 2014-10-21 02:34:39]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/657263002/80001

[commit-bot: I haz the power, 2014-10-21 03:10:37]

Committed patchset #5 (id:80001) as 184035