Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(488)

Issue 809323002: Correctly move script element to a detached document. (Closed)

Created:
6 years ago by sof
Modified:
5 years, 11 months ago
CC:
blink-reviews, blink-reviews-html_chromium.org, dglazkov+blink
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Target Ref:
refs/heads/master
Project:
blink
Visibility:
Public.

Description

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

Patch Set 1 #

Patch Set 2 : add test #

Total comments: 4

Messages

Total messages: 17 (4 generated)
sof
Please take a look.
6 years ago (2014-12-18 10:21:25 UTC) #2
marja
https://codereview.chromium.org/809323002/diff/20001/Source/core/html/HTMLScriptElement.cpp File Source/core/html/HTMLScriptElement.cpp (right): https://codereview.chromium.org/809323002/diff/20001/Source/core/html/HTMLScriptElement.cpp#newcode84 Source/core/html/HTMLScriptElement.cpp:84: contextDocument = &document(); In this case, is oldDocument also ...
6 years ago (2014-12-18 10:27:47 UTC) #3
sof
https://codereview.chromium.org/809323002/diff/20001/Source/core/html/HTMLScriptElement.cpp File Source/core/html/HTMLScriptElement.cpp (right): https://codereview.chromium.org/809323002/diff/20001/Source/core/html/HTMLScriptElement.cpp#newcode84 Source/core/html/HTMLScriptElement.cpp:84: contextDocument = &document(); On 2014/12/18 10:27:47, marja wrote: > ...
6 years ago (2014-12-18 10:29:37 UTC) #4
marja
lgtm, thx for fixing this so quickly.
6 years ago (2014-12-18 10:30:48 UTC) #5
sof
On 2014/12/18 10:30:48, marja wrote: > lgtm, thx for fixing this so quickly. thanks, this ...
6 years ago (2014-12-18 10:43:21 UTC) #6
marja
On 2014/12/18 10:43:21, sof wrote: > thanks, this extra document association of ScriptLoaders is turning ...
6 years ago (2014-12-18 10:48:49 UTC) #7
sof
+mkwst (would be good to send this along before going OOOish for a while :) ...
6 years ago (2014-12-18 12:52:02 UTC) #9
marja
adding jochen too, just in case...
6 years ago (2014-12-18 13:00:29 UTC) #11
jochen (gone - plz use gerrit)
lgtm
6 years ago (2014-12-18 13:05:41 UTC) #12
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/809323002/20001
6 years ago (2014-12-18 13:08:26 UTC) #14
commit-bot: I haz the power
Committed patchset #2 (id:20001) as https://src.chromium.org/viewvc/blink?view=rev&revision=187458
6 years ago (2014-12-18 13:11:25 UTC) #15
haraken
LGTM https://codereview.chromium.org/809323002/diff/20001/Source/core/html/HTMLScriptElement.cpp File Source/core/html/HTMLScriptElement.cpp (right): https://codereview.chromium.org/809323002/diff/20001/Source/core/html/HTMLScriptElement.cpp#newcode77 Source/core/html/HTMLScriptElement.cpp:77: if (!contextDocument) { I'd slightly prefer: RefPtrWillBeRawPtr<Document> contextDocument ...
6 years ago (2014-12-18 14:30:22 UTC) #16
sof
5 years, 11 months ago (2014-12-30 11:00:51 UTC) #17
Message was sent while issue was closed.
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/

Powered by Google App Engine
This is Rietveld 408576698