PlatformAppBrowserTest.Restrictions is flaky with NavigationMojoResponse |
||||||||
Issue description``` Findit (https://goo.gl/kROfz5) identified this CL at revision 543428 as the culprit for introducing flakiness in the tests as shown on: https://findit-for-me.appspot.com/waterfall/flake/flake-culprit?key=ag9zfmZpbmRpdC1mb3ItbWVyQwsSDEZsYWtlQ3VscHJpdCIxY2hyb21pdW0vZThkMTVmZmVhYzljMWNhMDAwNWRkNjQ5YzI3YzIwNTMzZTRmZjQ0ZQw ``` I can reproduce it locally on Linux. Note: This test is disabled with the NetworkService, it is probably related. See https://bugs.chromium.org/p/chromium/issues/detail?id=820995
,
Mar 16 2018
It looks like in a Chrome App, some javascript are injected, an event handler for window.onReadyStateChange is defined and it is used to disabled some javascript functionality.
_____________________________________________________________________________________
// Document instance properties that we wish to disable need to be set when
// the document begins loading, since only then will the "document" reference
// point to the page's document (it will be reset between now and then).
// We can't listen for the "readystatechange" event on the document (because
// the object that it's dispatched on doesn't exist yet), but we can instead
// do it at the window level in the capturing phase.
window.addEventListener('readystatechange', function(event) {
console.log("readystatechange = " + document.readyState);
if (document.readyState != 'loading')
return;
// Deprecated document properties from
// https://developer.mozilla.org/en/DOM/document.
// Disable document.all so that platform apps can not access.
delete Document.prototype.all
console.log("disable getter");
disableGetters(document, 'document',
['alinkColor', 'all', 'bgColor', 'fgColor', 'linkColor', 'vlinkColor'],
null, true);
}, true);
______________________________________________________________________________________
It becames flaky, because it is sometimes executed after the javascript test file have been loaded and have been executed.
Do you know if there are any constraints on when window.onReadyStateChange is called for the first time?
I don't really know if there is a bug in NavigationMojoResponse or if this script was relying on how things get scheduled in blink.
I wonder how critical it is to disable these deprecated getter.
+CC estade@ and rdevlin.cronin@: FYI. Let me know if you have any thoughts about it.
,
Mar 16 2018
FWIW:
# When it fails:
DocumentLoader::InstallNewDocument() causes
1) Document::SetReadyState("loading") to be called.
2) Dispatcher::DidCreateScriptContext() to be called. It injects and executes "platform_app.js"
It means the code for the "readystatechange" handler is not called.
# When it works, it is the same, except that Dispatcher::DidCreateScriptContext gets called before and I see this StackTrace():
#0 0x7f002b93985d base::debug::StackTrace::StackTrace()
#1 0x7f002b937d4c base::debug::StackTrace::StackTrace()
#2 0x00000530831c extensions::Dispatcher::DidCreateScriptContext()
#3 0x00000533bbeb extensions::ExtensionFrameHelper::DidCreateScriptContext()
#4 0x7f0025258a8b content::RenderFrameImpl::DidCreateScriptContext()
#5 0x7f0017cbff14 blink::LocalFrameClientImpl::DidCreateScriptContext()
#6 0x7f00173ee5d0 blink::LocalWindowProxy::Initialize()
#7 0x7f0017481375 blink::WindowProxy::InitializeIfNeeded()
#8 0x7f0017409f7a blink::WindowProxyManager::GetWindowProxy()
#9 0x7f0017dc0dcf blink::Frame::GetWindowProxy()
#10 0x7f0017de90e8 blink::LocalFrame::WindowProxy()
#11 0x7f001743e282 blink::ToV8ContextEvenIfDetached()
#12 0x7f001743e047 blink::ToScriptStateImpl()
#13 0x7f001743e302 blink::ToScriptState()
#14 0x7f001743e435 blink::ToScriptStateForMainWorld()
#15 0x7f0017e8dc93 blink::WebLocalFrameImpl::MainWorldScriptContext()
#16 0x000005277e64 extensions::AppWindowCustomBindings::GetFrame()
#17 0x000001d1e79d _ZN4base8internal13FunctorTraitsIM26BrowsingDataHelperCallbackIN7content21CacheStorageUsageInfoEEFvRKNSt3__14listIS4_NS6_9allocatorIS4_EEEEEvE6InvokeIPS5_JSC_EEEvSE_OT_DpOT0_
#18 0x000001d1e6ff _ZN4base8internal12InvokeHelperILb0EvE8MakeItSoIRKM26BrowsingDataHelperCallbackIN7content21CacheStorageUsageInfoEEFvRKNSt3__14listIS6_NS8_9allocatorIS6_EEEEEJPS7_SE_EEEvOT_DpOT0_
#19 0x000001d1e695 _ZN4base8internal7InvokerINS0_9BindStateIM26BrowsingDataHelperCallbackIN7content21CacheStorageUsageInfoEEFvRKNSt3__14listIS5_NS7_9allocatorIS5_EEEEEJNS0_17UnretainedWrapperIS6_EEEEEFvSD_EE7RunImplIRKSF_RKNS7_5tupleIJSH_EEEJLm0EEEEvOT_OT0_NS7_16integer_sequenceImJXspT1_EEEESD_
#20 0x000001d1e624 _ZN4base8internal7InvokerINS0_9BindStateIM26BrowsingDataHelperCallbackIN7content21CacheStorageUsageInfoEEFvRKNSt3__14listIS5_NS7_9allocatorIS5_EEEEEJNS0_17UnretainedWrapperIS6_EEEEEFvSD_EE3RunEPNS0_13BindStateBaseESD_
#21 0x000001e3fd1d _ZNKR4base17RepeatingCallbackIFvRKNSt3__112basic_stringIcNS1_11char_traitsIcEENS1_9allocatorIcEEEEEE3RunES9_
#22 0x0000053b57bf extensions::ObjectBackedNativeHandler::Router()
#23 0x7f002842d439 v8::internal::FunctionCallbackArguments::Call()
#24 0x7f002851c7a0 v8::internal::(anonymous namespace)::HandleApiCallHelper<>()
#25 0x7f002851aae9 v8::internal::Builtin_Impl_HandleApiCall()
#26 0x7f002851a52d v8::internal::Builtin_HandleApiCall()
,
Mar 19 2018
This code is still a bit unclear to me. I am not familiar with it. It looks like we don't have a strong guarantee that RenderFrameimpl::DidCreateScriptContext() will be called before the new Document is installed. If I look at the stacktrace above, it seems to happen lazily (See blink::WindowProxy::InitializeIfNeeded() above). I am not sure, but if it is the case, it means we can just update the script that gets "injected" such that it disable deprecated document attributes for the current document and for the next one. @estade: How critical it is to disable these deprecated attribute from "document" in the context of a Chrome App? I guess it is not very important, but I prefer to check.
,
Mar 19 2018
Unfortunately I'm not the one to ask. I see I reviewed a change 6 years ago[1] but I was only involved because I was the tree sheriff that day. Maybe Devlin or a Blink dev can provide a better answer. [1] https://chromium.googlesource.com/chromium/src/+/5a3aaecd4e74ac4ef92ac584ee7385f67715d450
,
Mar 19 2018
Thanks estade@. Sorry to interrupt you. Maybe japhet@ would know? Especially whether or not RenderFrameImpl::DidCreateScriptContext() must always be called before the new document is installed |DocumentLoader::InstallNewDocument()|.
,
Mar 21 2018
I took a closer look, I think I almost understand why there is a race condition.
1) When the Chrome App starts, a new windows is created.
_____________________________________________________________
chrome.app.runtime.onLaunched.addListener(function() {
chrome.app.window.create('main.html', {}, function () {});
});
_____________________________________________________________
the "app.window.create" function is executed on the browser process. See AppWindowCreateFunction::Run().
The Renderer process is expecting a response back, but the response is delayed until the navigation is told to commit.
_____________________________________________________________
// Delay sending the response until the newly created window has been told to
// navigate, and blink has been correctly initialized in the renderer.
// SetOnFirstCommitOrWindowClosedCallback() will respond asynchronously.
app_window->SetOnFirstCommitOrWindowClosedCallback(base::Bind(
&AppWindowCreateFunction::OnAppWindowReadyToCommitFirstNavigationOrClosed,
this, base::Passed(&result_arg)));
return RespondLater();
_____________________________________________________________
So the renderer process will receive in this order.
1) RenderFrameImpl::CommitNavigation()
2) Dispatcher::OnExtensionResponse()
1) Causes:
a) Creation of the DocumentLoader.
b) Waiting for reading the main response
c) Creation of the Document.
d) If needed, creation of the script context.
2) Causes:
a) "app.window.create" callback to be executed (See app_window_custom_bindings.js).
b) appWindowNavigates.GetFrames() is called.
c) If needed, creation of the script context.
The issue is that function called after 1) and 2) are interleaved.
The test pass when 1.c) happens before 2.c) and it fails otherwise.
I think it looks like this:
```
When it doesn't work.
┌────────┐ ┌───────┐
│Renderer│ │Browser│
└───┬────┘ └───┬───┘
│ │
│ app.window.create │
│───────────────────────────────>│
│ │
│ CommitNavigation() │
│<───────────────────────────────│
│ │
* Create DocumentLoader │
│ │
│ OnStartLoadingResponseBody() │
│<───────────────────────────────│
│ │
* Create Document │
* Create script context │
│ │
│ OnExtensionResponse() │
│<───────────────────────────────│
┌───┴────┐ ┌───┴───┐
│Renderer│ │Browser│
└────────┘ └───────┘
When it works.
┌────────┐ ┌───────┐
│Renderer│ │Browser│
└───┬────┘ └───┬───┘
│ │
│ app.window.create │
│───────────────────────────────>│
│ │
│ CommitNavigation() │
│<───────────────────────────────│
│ │
* Create DocumentLoader │
│ │
│ OnExtensionResponse() │
│<───────────────────────────────│
│ │
* Create script context │
│ │
│ OnStartLoadingResponseBody() │
│<───────────────────────────────│
│ │
* Create Document │
│ │
┌───┴────┐ ┌───┴───┐
│Renderer│ │Browser│
└────────┘ └───────┘
```
,
Mar 21 2018
Also FYI: I have a pending CL, https://chromium-review.googlesource.com/c/chromium/src/+/968871 It makes "platform_app.js" not depending on whether or not the document has already been created when the "platform_app.js" is executed. It is probably not enough. I am not feeling confident that OnExtensionResponse() for "app.window.create" may sometimes be received after the Document has been created.
,
Mar 22 2018
Do we know why OnextensionResponse is coming after CommitNavigation? Could we trigger it from ReadyToCommitNavigation instead? This should guarantee ordering.
,
Mar 22 2018
OnExtensionResponse() is sent after CommitNavigation() on purpose. It was done 2 years and 6 month ago by: https://codereview.chromium.org/1340163002 ``` @naskoe, creis, rdcronin: PTAL This CL fixes the issue of app windows with PlzNavigate that I mentionned by mail. The fix here is to delay sending the response to the creator of the app window until the navigation is ready to commit (which properly initializes the renderer). It is still guaranteed that the response will be sent before the OnLoad event, because the request from the renderer to get the body of the response will be blocked in the network stack until the callback has executed and it gets unblocked by the AppWindowContents. ```
,
Mar 22 2018
In app_window_contents.cc, requests that happens in the RFH are blocked while AppWindowContents::Impl::OnWindowReady() is not called. When the callback for chrome.app.window.create() is executed on the renderer. It will cause OnWindowReady() to be called and unblock the requests. Without NavigationMojoResponse, a new request was made to retrieve the main response body. So it was blocked and unblocked. The onload event was guaranteed to be dispatched after receiving the chrome.app.window.create callback. With NavigationMojoResponse, there are no more additional request. That's the issue. However, I think we are still good. In a Chrome Apps, inline script are not allowed. If there are javascript, it must be in a separate resource. This one will be blocked. So as long as there are a script, the onload event can't happen after the callback has been executed. Finally, the case where there are no scripts doesn't matter. Am I right? What do you think? In this case, it means we can simply apply https://chromium-review.googlesource.com/c/chromium/src/+/968871 and everything will be okay.
,
Mar 23 2018
Hmmm, what I am saying doesn't hold. The background app can create an onload event handler for the window. If the document doesn't contains any subresources (scripts, ...), then the onload event might be already triggered. I made a new test here: https://chromium-review.googlesource.com/c/chromium/src/+/978210 It doesn't work with NavigationMojoResponse. I think we really need to find a way to prevent the new document to load before the chrome.app.window.create callback has been called. It's probably the only way to fix the issue.
,
Mar 30 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/5ad360d71877f1b104c54e785f4e550319aa4d38 commit 5ad360d71877f1b104c54e785f4e550319aa4d38 Author: arthursonzogni <arthursonzogni@chromium.org> Date: Fri Mar 30 23:10:05 2018 [Chrome App] Fix race condition with new window loading. In the context of a Chrome Apps, a background script can creates new windows using chrome.app.window.create(): See https://developer.chrome.com/apps/app_window#method-create It takes a javascript callback. Once the new window is created, the window is given to the callback such that the background script can make use of it. The callback is called after the document has been created, but it must also be called before it has finished loading. The last point is documented in the link above. To schedule thing properly, |BlockRequestsForRoute| and |ResumeBlockedRequestsForRoute| functions in the ResourceDispatcherHost were used. That way, a DocumentLoader was created, but was not able to fetch any data. It gives time for the Chrome Apps to setup everything and call the callback. There are two issues with this approach. * It doesn't work with the network service, because the network service doesn't use the resource dispatcher host. * It doesn't work with NavigationMojoResponse, because the request has already been made and no additionnal requests are sent to retrieve the main resource response. This CL replaces this mechanism by using DocumentLoader::ResumeParser() and DocumentLoader::BlockParser() instead. TBR=benwells@chromium.org Bug: 822650 Change-Id: I34071127e1c9598980e8a4afd9367b737da2878c Reviewed-on: https://chromium-review.googlesource.com/982059 Reviewed-by: Daniel Cheng <dcheng@chromium.org> Reviewed-by: Devlin <rdevlin.cronin@chromium.org> Reviewed-by: Arthur Sonzogni <arthursonzogni@chromium.org> Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org> Cr-Commit-Position: refs/heads/master@{#547309} [modify] https://crrev.com/5ad360d71877f1b104c54e785f4e550319aa4d38/chrome/browser/apps/app_browsertest.cc [modify] https://crrev.com/5ad360d71877f1b104c54e785f4e550319aa4d38/extensions/browser/app_window/app_window.cc [modify] https://crrev.com/5ad360d71877f1b104c54e785f4e550319aa4d38/extensions/browser/app_window/app_window.h [modify] https://crrev.com/5ad360d71877f1b104c54e785f4e550319aa4d38/extensions/browser/app_window/app_window_contents.cc [modify] https://crrev.com/5ad360d71877f1b104c54e785f4e550319aa4d38/extensions/browser/app_window/app_window_contents.h [modify] https://crrev.com/5ad360d71877f1b104c54e785f4e550319aa4d38/extensions/browser/app_window/test_app_window_contents.cc [modify] https://crrev.com/5ad360d71877f1b104c54e785f4e550319aa4d38/extensions/browser/app_window/test_app_window_contents.h [modify] https://crrev.com/5ad360d71877f1b104c54e785f4e550319aa4d38/extensions/renderer/app_window_custom_bindings.cc [modify] https://crrev.com/5ad360d71877f1b104c54e785f4e550319aa4d38/extensions/renderer/app_window_custom_bindings.h [modify] https://crrev.com/5ad360d71877f1b104c54e785f4e550319aa4d38/extensions/renderer/extension_frame_helper.cc [modify] https://crrev.com/5ad360d71877f1b104c54e785f4e550319aa4d38/extensions/renderer/extension_frame_helper.h [modify] https://crrev.com/5ad360d71877f1b104c54e785f4e550319aa4d38/extensions/renderer/resources/app_window_custom_bindings.js [modify] https://crrev.com/5ad360d71877f1b104c54e785f4e550319aa4d38/extensions/renderer/resources/platform_app.js [modify] https://crrev.com/5ad360d71877f1b104c54e785f4e550319aa4d38/third_party/WebKit/Source/core/exported/WebDocumentLoaderImpl.cpp [modify] https://crrev.com/5ad360d71877f1b104c54e785f4e550319aa4d38/third_party/WebKit/Source/core/exported/WebDocumentLoaderImpl.h [modify] https://crrev.com/5ad360d71877f1b104c54e785f4e550319aa4d38/third_party/WebKit/Source/core/loader/DocumentLoader.cpp [modify] https://crrev.com/5ad360d71877f1b104c54e785f4e550319aa4d38/third_party/WebKit/public/web/WebDocumentLoader.h
,
Apr 3 2018
I am requesting a merge request for M66 beta: https://chromium-review.googlesource.com/982059 It fixes a race condition with Chrome Apps. The race condition occurs only when NavigationMojoResponse is enabled (currently enabled at 50% on M66 beta). It has been on Canary since 67.0.3385.0 [2018-03-30]
,
Apr 3 2018
This bug requires manual review: We are only 13 days from stable. Please contact the milestone owner if you have questions. Owners: cmasso@(Android), cmasso@(iOS), josafat@(ChromeOS), abdulsyed@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Apr 3 2018
In case it is accepted, I prepared the cherry-pick here: https://chromium-review.googlesource.com/c/chromium/src/+/992414
,
Apr 3 2018
Approving merge for M66. Branch:3359
,
Apr 3 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/5973f6cfd5a6277b17cc0497b937193921ff29d1 commit 5973f6cfd5a6277b17cc0497b937193921ff29d1 Author: arthursonzogni <arthursonzogni@chromium.org> Date: Tue Apr 03 17:41:56 2018 [Chrome App] Fix race condition with new window loading. [M66 merge] In the context of a Chrome Apps, a background script can creates new windows using chrome.app.window.create(): See https://developer.chrome.com/apps/app_window#method-create It takes a javascript callback. Once the new window is created, the window is given to the callback such that the background script can make use of it. The callback is called after the document has been created, but it must also be called before it has finished loading. The last point is documented in the link above. To schedule thing properly, |BlockRequestsForRoute| and |ResumeBlockedRequestsForRoute| functions in the ResourceDispatcherHost were used. That way, a DocumentLoader was created, but was not able to fetch any data. It gives time for the Chrome Apps to setup everything and call the callback. There are two issues with this approach. * It doesn't work with the network service, because the network service doesn't use the resource dispatcher host. * It doesn't work with NavigationMojoResponse, because the request has already been made and no additionnal requests are sent to retrieve the main resource response. This CL replaces this mechanism by using DocumentLoader::ResumeParser() and DocumentLoader::BlockParser() instead. TBR=benwells@chromium.org Bug: 822650 Change-Id: I34071127e1c9598980e8a4afd9367b737da2878c Reviewed-on: https://chromium-review.googlesource.com/982059 Reviewed-by: Daniel Cheng <dcheng@chromium.org> Reviewed-by: Devlin <rdevlin.cronin@chromium.org> Reviewed-by: Arthur Sonzogni <arthursonzogni@chromium.org> Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#547309}(cherry picked from commit 5ad360d71877f1b104c54e785f4e550319aa4d38) Reviewed-on: https://chromium-review.googlesource.com/992414 Reviewed-by: Nasko Oskov <nasko@chromium.org> Cr-Commit-Position: refs/branch-heads/3359@{#552} Cr-Branched-From: 66afc5e5d10127546cc4b98b9117aff588b5e66b-refs/heads/master@{#540276} [modify] https://crrev.com/5973f6cfd5a6277b17cc0497b937193921ff29d1/extensions/browser/app_window/app_window.cc [modify] https://crrev.com/5973f6cfd5a6277b17cc0497b937193921ff29d1/extensions/browser/app_window/app_window.h [modify] https://crrev.com/5973f6cfd5a6277b17cc0497b937193921ff29d1/extensions/browser/app_window/app_window_contents.cc [modify] https://crrev.com/5973f6cfd5a6277b17cc0497b937193921ff29d1/extensions/browser/app_window/app_window_contents.h [modify] https://crrev.com/5973f6cfd5a6277b17cc0497b937193921ff29d1/extensions/browser/app_window/test_app_window_contents.cc [modify] https://crrev.com/5973f6cfd5a6277b17cc0497b937193921ff29d1/extensions/browser/app_window/test_app_window_contents.h [modify] https://crrev.com/5973f6cfd5a6277b17cc0497b937193921ff29d1/extensions/renderer/app_window_custom_bindings.cc [modify] https://crrev.com/5973f6cfd5a6277b17cc0497b937193921ff29d1/extensions/renderer/app_window_custom_bindings.h [modify] https://crrev.com/5973f6cfd5a6277b17cc0497b937193921ff29d1/extensions/renderer/extension_frame_helper.cc [modify] https://crrev.com/5973f6cfd5a6277b17cc0497b937193921ff29d1/extensions/renderer/extension_frame_helper.h [modify] https://crrev.com/5973f6cfd5a6277b17cc0497b937193921ff29d1/extensions/renderer/resources/app_window_custom_bindings.js [modify] https://crrev.com/5973f6cfd5a6277b17cc0497b937193921ff29d1/extensions/renderer/resources/platform_app.js [modify] https://crrev.com/5973f6cfd5a6277b17cc0497b937193921ff29d1/third_party/WebKit/Source/core/exported/WebDocumentLoaderImpl.cpp [modify] https://crrev.com/5973f6cfd5a6277b17cc0497b937193921ff29d1/third_party/WebKit/Source/core/exported/WebDocumentLoaderImpl.h [modify] https://crrev.com/5973f6cfd5a6277b17cc0497b937193921ff29d1/third_party/WebKit/Source/core/loader/DocumentLoader.cpp [modify] https://crrev.com/5973f6cfd5a6277b17cc0497b937193921ff29d1/third_party/WebKit/public/web/WebDocumentLoader.h
,
Apr 4 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/6e1e31edd72513c2b6cabcc7f7bae4a8a23af056 commit 6e1e31edd72513c2b6cabcc7f7bae4a8a23af056 Author: arthursonzogni <arthursonzogni@chromium.org> Date: Wed Apr 04 12:23:19 2018 Use a counter for DocumentLoader::BlockParser(). Replace usage of a bool by a counter to control whether or not the parser is allowed to read new data or not. Since WebDocumentLoader::BlockParser() is public, they may be several components trying to pause the parser at the same time. Using a counter make it possible to wait for all of them to call WebDocumentLoader::ResumeParser(). This is a follow up for: https://chromium-review.googlesource.com/982059 Bug: 822650 Change-Id: Id6d42f911f4b6e69ca942d8bf19acd296327d419 Reviewed-on: https://chromium-review.googlesource.com/992033 Reviewed-by: Daniel Cheng <dcheng@chromium.org> Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org> Cr-Commit-Position: refs/heads/master@{#548028} [modify] https://crrev.com/6e1e31edd72513c2b6cabcc7f7bae4a8a23af056/third_party/WebKit/Source/core/loader/DocumentLoader.cpp [modify] https://crrev.com/6e1e31edd72513c2b6cabcc7f7bae4a8a23af056/third_party/WebKit/Source/core/loader/DocumentLoader.h [modify] https://crrev.com/6e1e31edd72513c2b6cabcc7f7bae4a8a23af056/third_party/WebKit/public/web/WebDocumentLoader.h |
||||||||
►
Sign in to add a comment |
||||||||
Comment 1 by bugdroid1@chromium.org
, Mar 16 2018