Correctly move script element to a detached document.

Correctly move script element to a detached document.

If a script element ends up being moved over to a new document's tree,
its script loader is re-associated with that document's script runner
if the load is still pending. That association of script runner and
loader needs to reflect document association of the element itself,
otherwise completion of script loading and any later movement of the
element to another document will go wrong.

R=marja,jochen
BUG=443115
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=187458

[sof, 2014-12-18 10:21:24]

sigbjornf@opera.com changed reviewers:
+ haraken@chromium.org, kouhei@chromium.org, marja@chromium.org

[sof, 2014-12-18 10:21:25]

Please take a look.

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

https://codereview.chromium.org/809323002/diff/20001/Source/core/html/HTMLScr...
File Source/core/html/HTMLScriptElement.cpp (right):

https://codereview.chromium.org/809323002/diff/20001/Source/core/html/HTMLScr...
Source/core/html/HTMLScriptElement.cpp:84: contextDocument = &document();
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]

https://codereview.chromium.org/809323002/diff/20001/Source/core/html/HTMLScr...
File Source/core/html/HTMLScriptElement.cpp (right):

https://codereview.chromium.org/809323002/diff/20001/Source/core/html/HTMLScr...
Source/core/html/HTMLScriptElement.cpp:84: contextDocument = &document();
On 2014/12/18 10:27:47, marja wrote:
> In this case, is oldDocument also the same as document() ? If yes, isn't the
> movePendingAsyncScript below NOOP? If no, why not?

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

[marja, 2014-12-18 10:30:48]

lgtm, thx for fixing this so quickly.

[sof, 2014-12-18 10:43:21]

On 2014/12/18 10:30:48, marja wrote:
> lgtm, thx for fixing this so quickly.

thanks, this extra document association of ScriptLoaders is turning out brittle,
perhaps something that can improved and reworked a bit later.

haraken@ and/or kouhei@: agree as core/ owners?

[marja, 2014-12-18 10:48:49]

On 2014/12/18 10:43:21, sof wrote:
> thanks, this extra document association of ScriptLoaders is turning out
brittle,
> perhaps something that can improved and reworked a bit later.

I was also wondering whether the relevant ASSERTs in ScriptRunner / Loader
should be turned into RELEASE_ASSERTs, so that we would at least crash in a
controlled way in these situations. Maybe something to fix next year? :)

[sof, 2014-12-18 12:52:01]

sigbjornf@opera.com changed reviewers:
+ mkwst@chromium.org

[sof, 2014-12-18 12:52:02]

+mkwst (would be good to send this along before going OOOish for a while :) )

[marja, 2014-12-18 13:00:28]

marja@chromium.org changed reviewers:
+ jochen@chromium.org

[marja, 2014-12-18 13:00:29]

adding jochen too, just in case...

[jochen (gone - plz use gerrit), 2014-12-18 13:05:41]

lgtm

[sof, 2014-12-18 13:07:35]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2014-12-18 13:08:26]

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

[commit-bot: I haz the power, 2014-12-18 13:11:25]

Committed patchset #2 (id:20001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=187458

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

LGTM

https://codereview.chromium.org/809323002/diff/20001/Source/core/html/HTMLScr...
File Source/core/html/HTMLScriptElement.cpp (right):

https://codereview.chromium.org/809323002/diff/20001/Source/core/html/HTMLScr...
Source/core/html/HTMLScriptElement.cpp:77: if (!contextDocument) {

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]

https://codereview.chromium.org/809323002/diff/20001/Source/core/html/HTMLScr...
File Source/core/html/HTMLScriptElement.cpp (right):

https://codereview.chromium.org/809323002/diff/20001/Source/core/html/HTMLScr...
Source/core/html/HTMLScriptElement.cpp:77: if (!contextDocument) {
On 2014/12/18 14:30:22, haraken wrote:
> 
> 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.

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/