Correctly move script element to a detached document.

Source/core/html/HTMLScriptElement.cpp

 /*
  * Copyright (C) 1999 Lars Knoll (knoll@kde.org)
  *           (C) 1999 Antti Koivisto (koivisto@kde.org)
  *           (C) 2001 Dirk Mueller (mueller@kde.org)
  * Copyright (C) 2003, 2004, 2005, 2006, 2007, 2008 Apple Inc. All rights reserved.
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Library General Public
  * License as published by the Free Software Foundation; either
  * version 2 of the License, or (at your option) any later version.
@@ skipping 55 matching lines @@
 }
 
 void HTMLScriptElement::childrenChanged(const ChildrenChange& change)
 {
     HTMLElement::childrenChanged(change);
     m_loader->childrenChanged();
 }
 
 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);
 }
 
 void HTMLScriptElement::parseAttribute(const QualifiedName& name, const AtomicString& value)
 {
     if (name == srcAttr)
         m_loader->handleSourceAttribute(value);
     else if (name == asyncAttr)
         m_loader->handleAsyncAttribute();
     else
@@ skipping 124 matching lines @@
     return adoptRefWillBeNoop(new HTMLScriptElement(document(), false, m_loader->alreadyStarted()));
 }
 
 void HTMLScriptElement::trace(Visitor* visitor)
 {
     visitor->trace(m_loader);
     HTMLElement::trace(visitor);
 }
 
 }