Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
313173012:
HTMLParser should use current value of scriptEnabled flag (Closed)
|
Created:
6 years, 6 months ago by mmal Modified:
6 years, 5 months ago CC:
arv+blink, blink-reviews, blink-reviews-bindings_chromium.org, blink-reviews-html_chromium.org, dglazkov+blink Base URL:
https://chromium.googlesource.com/chromium/blink.git@WebKit-ADD Visibility:
Public. |
DescriptionHTMLParser should use current value of scriptEnabled flag
This CL adds caching of scriptEnabled setting to Document.
When page is reloaded we create new parser throught DocumentLoader::ensureWriter
and set scriptEnabled flag. ContentSettingsObserver::allowScript method is
responsible for checking settings but it uses cache. Cache is cleared on
DidCommitProvisionalLoad but parser is already created before that. However we
can't clear cache earlier because unload events for old page might be fired and
it depends on old page settings (cached).
BUG=232410
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=177664
Patch Set 1 : New approach #
Total comments: 6
Patch Set 2 : #Patch Set 3 : lazy init #Patch Set 4 : Rebase #Patch Set 5 : Tests fix - make use of window.open() #
Total comments: 1
Patch Set 6 : #
Messages
Total messages: 38 (0 generated)
+adam/eric for parser
https://codereview.chromium.org/313173012/diff/1/Source/bindings/v8/ScriptCon... File Source/bindings/v8/ScriptController.h (right): https://codereview.chromium.org/313173012/diff/1/Source/bindings/v8/ScriptCon... Source/bindings/v8/ScriptController.h:67: AboutToParseHTML not lgtm It's not sensible to add this enum value. ScriptController shouldn't know anything about how the HTML parser works.
Also, your CL is missing tests.
On 2014/06/06 17:21:13, abarth wrote: > https://codereview.chromium.org/313173012/diff/1/Source/bindings/v8/ScriptCon... > File Source/bindings/v8/ScriptController.h (right): > > https://codereview.chromium.org/313173012/diff/1/Source/bindings/v8/ScriptCon... > Source/bindings/v8/ScriptController.h:67: AboutToParseHTML > not lgtm > > It's not sensible to add this enum value. ScriptController shouldn't know > anything about how the HTML parser works. Problem is that permission for executing scripts is not the same as permission to parsing script related tags both conceptually and in practice (the latter caused by caching). Alternatively I can add methods like allowScriptNoCache, canExecuteScriptsNoCache (or add bool useCache). But again why HTMLParserOptions or V8 should know anything about controller using cache? Someone must know something. I think it's better that controller knows the context than its clients knowing about caching.
On 2014/06/11 at 10:28:00, mmaliszkiewicz wrote: > Problem is that permission for executing scripts is not the same as permission to parsing script related tags both conceptually and in practice (the latter caused by caching). I'm not sure I understand. Can you give me some examples that illustrate the difference? > Alternatively I can add methods like allowScriptNoCache, canExecuteScriptsNoCache (or add bool useCache). But again why HTMLParserOptions or V8 should know anything about controller using cache? Someone must know something. I think it's better that controller knows the context than its clients knowing about caching. It's hard for me to suggest a solution because I don't understand the problem. It's unclear to me what allowScriptNoCache means.
From the description, this sounds like a bug in the embedder. We're asking whether script is allowed, and it's not giving us the correct answer.
On 2014/06/11 19:35:38, abarth wrote: > From the description, this sounds like a bug in the embedder. We're asking > whether script is allowed, and it's not giving us the correct answer. Actually an example of this difference is in CL description. It looks like this: 1) User allow scripts 2) We load page A 3) User disallow scripts 4) We load page B (the same tab) a) page be is being parsed and asks ScriptController about script execution, and answer should be DISALLOW b) page be is being unloaded, V8 needs to know whether call or not onUnload, onBeforeunload and ScriptController should return ALLOW despite the fact that actually user settings at this moment are set to DISALLOW scripts.
> a) page be is being parsed and asks ScriptController about script execution, > and answer should be DISALLOW > b) page be is being unloaded, V8 needs to know whether call or not onUnload, > onBeforeunload and ScriptController should return ALLOW despite the fact that > actually user settings at this moment are set to DISALLOW scripts. Should be: a) page B is being ... b) page A is being ...
So, the intent of the setting is that it takes effect on the next page load? Perhaps we should move the cache from being in the embedder to being inside Blink. Then we store the cache on the correct object (e.g., ExecutionContext) and it will be cached for exactly the correct amount of time.
https://codereview.chromium.org/313173012/diff/1/Source/core/html/parser/HTML... File Source/core/html/parser/HTMLParserOptions.cpp (right): https://codereview.chromium.org/313173012/diff/1/Source/core/html/parser/HTML... Source/core/html/parser/HTMLParserOptions.cpp:40: scriptEnabled = frame && frame->script().canExecuteScripts(AboutToParseHTML); I agree with abarth. If you want to keep this value around for a differnet lifetime then I would just cache it somewehre else and have HTMLParserOptions read from that other place instead of adding this strnage enum value.
On 2014/06/17 21:24:08, eseidel wrote: > https://codereview.chromium.org/313173012/diff/1/Source/core/html/parser/HTML... > File Source/core/html/parser/HTMLParserOptions.cpp (right): > > https://codereview.chromium.org/313173012/diff/1/Source/core/html/parser/HTML... > Source/core/html/parser/HTMLParserOptions.cpp:40: scriptEnabled = frame && > frame->script().canExecuteScripts(AboutToParseHTML); > I agree with abarth. If you want to keep this value around for a differnet > lifetime then I would just cache it somewehre else and have HTMLParserOptions > read from that other place instead of adding this strnage enum value. Couldn't cache be held inside ScriptController? I need LocalFrame to set it and I'm not sure how get it while being inside ExecutionContext.
On 2014/06/18 at 14:01:46, mmaliszkiewicz wrote: > Couldn't cache be held inside ScriptController? I need LocalFrame to set it and I'm not sure how get it while being inside ExecutionContext. Nope. ScriptController is re-used for different documents. You need to store the data in ExecutionContext.
How about this approach?
This approach is great! Some minor questions below. https://codereview.chromium.org/313173012/diff/20001/Source/core/dom/Executio... File Source/core/dom/ExecutionContext.cpp (right): https://codereview.chromium.org/313173012/diff/20001/Source/core/dom/Executio... Source/core/dom/ExecutionContext.cpp:67: , m_scriptEnabled(false) What about other subclasses of ExecutionContext? For example, doesn't WorkerGlobalScope want to enable scripts? https://codereview.chromium.org/313173012/diff/20001/Source/core/dom/Executio... File Source/core/dom/ExecutionContext.h (right): https://codereview.chromium.org/313173012/diff/20001/Source/core/dom/Executio... Source/core/dom/ExecutionContext.h:87: void initScriptEnabled() { } This function shouldn't be needed. What breaks if you delete it? https://codereview.chromium.org/313173012/diff/20001/Source/core/loader/Docum... File Source/core/loader/DocumentLoader.cpp (right): https://codereview.chromium.org/313173012/diff/20001/Source/core/loader/Docum... Source/core/loader/DocumentLoader.cpp:504: m_frame->document()->initScriptEnabled(); Can we move this call into Document::Document, right after we call initSecurityContext ?
https://codereview.chromium.org/313173012/diff/20001/Source/core/dom/Executio... File Source/core/dom/ExecutionContext.cpp (right): https://codereview.chromium.org/313173012/diff/20001/Source/core/dom/Executio... Source/core/dom/ExecutionContext.cpp:67: , m_scriptEnabled(false) On 2014/06/26 19:53:37, abarth wrote: > What about other subclasses of ExecutionContext? For example, doesn't > WorkerGlobalScope want to enable scripts? WorkerGlobalScope seems to have its own WorkerScriptController. Only LocalFrame instantiates ScriptController. Maybe bool m_scriptEnabled belongs more to Document than to ExecutionContext? https://codereview.chromium.org/313173012/diff/20001/Source/core/dom/Executio... File Source/core/dom/ExecutionContext.h (right): https://codereview.chromium.org/313173012/diff/20001/Source/core/dom/Executio... Source/core/dom/ExecutionContext.h:87: void initScriptEnabled() { } On 2014/06/26 19:53:37, abarth wrote: > This function shouldn't be needed. What breaks if you delete it? Done. https://codereview.chromium.org/313173012/diff/20001/Source/core/loader/Docum... File Source/core/loader/DocumentLoader.cpp (right): https://codereview.chromium.org/313173012/diff/20001/Source/core/loader/Docum... Source/core/loader/DocumentLoader.cpp:504: m_frame->document()->initScriptEnabled(); On 2014/06/26 19:53:37, abarth wrote: > Can we move this call into Document::Document, right after we call > initSecurityContext ? I'm afraid we can't. Causes crash on WebSecurityOrigin::toString(). ContentSettingsObserver uses WebDocument, doesn't it require (at least) Document object to be fully created?
On 2014/06/27 at 19:39:16, mmaliszkiewicz wrote: > WorkerGlobalScope seems to have its own WorkerScriptController. Only LocalFrame instantiates ScriptController. Maybe bool m_scriptEnabled belongs more to Document than to ExecutionContext? Yes, that makes good sense. > https://codereview.chromium.org/313173012/diff/20001/Source/core/loader/Docum... > Source/core/loader/DocumentLoader.cpp:504: m_frame->document()->initScriptEnabled(); > On 2014/06/26 19:53:37, abarth wrote: > > Can we move this call into Document::Document, right after we call > > initSecurityContext ? > > I'm afraid we can't. Causes crash on WebSecurityOrigin::toString(). > ContentSettingsObserver uses WebDocument, doesn't it require (at least) Document object to be fully created? Hum... Maybe we can cache the value the first time it's queried? You're initializing the value in an aribtrary place that isn't necessarily visited by every document.
Perfect! LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mmaliszkiewicz@opera.com/313173012/60001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_blink_dbg on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...) linux_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/1...) mac_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/13616)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/1...)
On 2014/06/30 19:29:05, I haz the power (commit-bot) wrote: > Try jobs failed on following builders: > linux_blink_rel on tryserver.blink > (http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/1...) Did something go wrong?
On 2014/07/04 at 11:45:38, mmaliszkiewicz wrote: > On 2014/06/30 19:29:05, I haz the power (commit-bot) wrote: > > Try jobs failed on following builders: > > linux_blink_rel on tryserver.blink > > (http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/1...) > > Did something go wrong? Looks like your CL fails two tests.
On 2014/07/04 15:32:01, abarth wrote: > https://storage.googleapis.com/chromium-layout-test-archives/linux_blink_rel/... These tests are disabling javascript through javascript, we need to reload the page.
On 2014/07/04 at 19:35:26, mmaliszkiewicz wrote: > On 2014/07/04 15:32:01, abarth wrote: > > https://storage.googleapis.com/chromium-layout-test-archives/linux_blink_rel/... > > These tests are disabling javascript through javascript, we need to reload the page. One thing you can do is move the test into a popup window. That's easier to reload and control.
On 2014/07/04 19:48:06, abarth wrote: > On 2014/07/04 at 19:35:26, mmaliszkiewicz wrote: > > On 2014/07/04 15:32:01, abarth wrote: > > > > https://storage.googleapis.com/chromium-layout-test-archives/linux_blink_rel/... > > > > These tests are disabling javascript through javascript, we need to reload the > page. > > One thing you can do is move the test into a popup window. That's easier to > reload and control. How about Patch Set 4 I uploaded?
I deleted last tests fix (btw: there are tests which try to enable javascript through javascript - odd) and I made new one.
LGTM https://codereview.chromium.org/313173012/diff/200001/LayoutTests/fast/harnes... File LayoutTests/fast/harness/override-preferences.html (right): https://codereview.chromium.org/313173012/diff/200001/LayoutTests/fast/harnes... LayoutTests/fast/harness/override-preferences.html:14: win.onload = function(){testRunner.notifyDone()}; win.onload = function() { testRunner.notifyDone() };
The CQ bit was checked by mmaliszkiewicz@opera.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mmaliszkiewicz@opera.com/313173012/220001
Message was sent while issue was closed.
Change committed as 177664
Message was sent while issue was closed.
A revert of this CL has been created in https://codereview.chromium.org/375263002/ by yutak@chromium.org. The reason for reverting is: This patch *appears* to break AwSettingsTest and AwContentsClientShouldOverrideUrlLoadingTest: http://build.chromium.org/p/chromium.webkit/builders/Android%20Tests%20(dbg) This is my wild guess but it might be possible that this patch is the cause, since the tests in question call setJavaScriptEnabled(true). Apologies if my guess is wrong. I'll revert the revert if it turned out that I'm wrong..
Message was sent while issue was closed.
On 2014/07/09 09:20:43, Yuta Kitamura wrote: > A revert of this CL has been created in > https://codereview.chromium.org/375263002/ by mailto:yutak@chromium.org. > > The reason for reverting is: This patch *appears* to break AwSettingsTest and > AwContentsClientShouldOverrideUrlLoadingTest: > http://build.chromium.org/p/chromium.webkit/builders/Android%20Tests%20(dbg) > > This is my wild guess but it might be possible > that this patch is the cause, since the tests in > question call setJavaScriptEnabled(true). > > Apologies if my guess is wrong. I'll revert the > revert if it turned out that I'm wrong.. Most likely you are right. Are only tests broken or some functionality as well? I think main difference about this patch is that now you can't change document's javascript on/off state without recreating the document (i.e. by reloading the page). This behavior is closer to what browser actually do but previously it was done in chromium layer so WebKit tests simply didn't take it into account. I'm not familiar with Android tests, could you try to repair it? If they use javascript then you can rewrite it in a manner I dealt with LayoutTests. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
