Correctly move script element to a detached document.

Source/core/html/HTMLScriptElement.cpp

Index: Source/core/html/HTMLScriptElement.cpp
diff --git a/Source/core/html/HTMLScriptElement.cpp b/Source/core/html/HTMLScriptElement.cpp
index 6bfccd19b5be16247a313ae4f1ef572071062f46..bce71bdbcb9686e604a03408401a08950b6eeb5f 100644
--- a/Source/core/html/HTMLScriptElement.cpp
+++ b/Source/core/html/HTMLScriptElement.cpp
@@ -73,8 +73,17 @@ void HTMLScriptElement::childrenChanged(const ChildrenChange& change)
 
 void HTMLScriptElement::didMoveToNewDocument(Document& oldDocument)
 {
-    if (RefPtrWillBeRawPtr<Document> contextDocument = document().contextDocument().get())
-        oldDocument.scriptRunner()->movePendingAsyncScript(contextDocument->scriptRunner(), m_loader.get());
+    RefPtrWillBeRawPtr<Document> contextDocument = document().contextDocument().get();
+    if (!contextDocument) {

[haraken, 2014/12/18 14:30:22]

I'd slightly prefer:

RefPtrWillBeRawPtr<Document> contextDocument = nullptr;
if (document().frame()) {  // Frame-attached document.
  ASSERT(document().contextDocument());
  contextDocument = document().contextDocument();
} else {  // Frame-detached document.
  ASSERT(!document().contextDocument());
  contextDocument = &document();
}

to make it clear we need separate handling depending on whether the document is
detached from a frame or not.

[sof, 2014/12/30 11:00:51]

Thanks for bringing this up. That wouldn't be quite correct, as the context
document might be explicitly supplied for the document when created. In which
case it would be returned from contextDocument() (iff that weakly referenced
document is otherwise kept alive), irrespective of the script element's frame
attachment status.  

That detail wasn't made clear by the comment below; tried to improve it in
https://codereview.chromium.org/827233002/

+        ASSERT(!document().frame());
+        // A frame-detached document is handled as having no context
+        // document - it would be the document if not detached. The
+        // newly moved script element needs to be the latter here as
+        // the script loader for the pending script must also move to
+        // reside with that document and its script runner.
+        contextDocument = &document();

[marja, 2014/12/18 10:27:47]

In this case, is oldDocument also the same as document() ? If yes, isn't the
movePendingAsyncScript below NOOP? If no, why not?

[sof, 2014/12/18 10:29:37]

No, higher up in the node adoption handling checks that they're not equal.

+    }
+    oldDocument.scriptRunner()->movePendingAsyncScript(contextDocument->scriptRunner(), m_loader.get());
     HTMLElement::didMoveToNewDocument(oldDocument);
 }