Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2892353002:
WebUI: Fix crash while loading scripts for webview.[add/remove]ContentScripts API. (Closed)
|
Created:
3 years, 7 months ago by karandeepb Modified:
3 years, 7 months ago Reviewers:
lazyboy CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionWebUI: Fix crash while loading scripts for webview.[add/remove]ContentScripts API.
WebUIUserScriptLoader is used by the DeclarativeUserScriptMaster to load scripts
in webui through the declarative webview.[add/remove]ContentScripts API. As per
crash reports, the cached UserScriptRenderInfo may not correspond to an alive
render process host. This can happen since UserScriptLoader::LoadScripts may not
be synchronous with UserScriptLoader::AddScripts.
This CL tries to fix the crash by checking whether the cached
UserScriptRenderInfo corresponds to a live RenderProcessHost. If it doesn't the
corresponding script is not loaded.
BUG=720331
Review-Url: https://codereview.chromium.org/2892353002
Cr-Commit-Position: refs/heads/master@{#474457}
Committed: https://chromium.googlesource.com/chromium/src/+/017234bc0a7505f2a773352e1d38090847aee163
Patch Set 1 #
Total comments: 2
Patch Set 2 : Add test. #Patch Set 3 : Remove test and add TODO #
Messages
Total messages: 24 (19 generated)
Description was changed from ========== WebUI: Fix crash while loading scripts for webview.[add/remove]ContentScripts API. WebUIUserScriptLoader is used by the DeclarativeUserScriptMaster to load scripts in webui through the declarative webview.[add/remove]ContentScripts API. As per crash reports, the cached UserScriptRenderInfo may not correspond to an alive render process host. This can happen since UserScriptLoader::LoadScripts may not be synchronous with UserScriptLoader::AddScripts. This CL tries to tentatively fix the crash by checking whether the cached UserScriptRenderInfo corresponds to a live RenderProcessHost. If it doesn't the corresponding script is not loaded. BUG=720331 ========== to ========== WebUI: Fix crash while loading scripts for webview.[add/remove]ContentScripts API. WebUIUserScriptLoader is used by the DeclarativeUserScriptMaster to load scripts in webui through the declarative webview.[add/remove]ContentScripts API. As per crash reports, the cached UserScriptRenderInfo may not correspond to an alive render process host. This can happen since UserScriptLoader::LoadScripts may not be synchronous with UserScriptLoader::AddScripts. This CL tries to fix the crash by checking whether the cached UserScriptRenderInfo corresponds to a live RenderProcessHost. If it doesn't the corresponding script is not loaded. BUG=720331 ==========
The CQ bit was checked by karandeepb@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
karandeepb@chromium.org changed reviewers: + lazyboy@chromium.org
PTAL Istiaque. Don't have a repro case, but think this should fix it. I am not clear on what can cause the RenderProcessHost to not be alive.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
> I am not clear on what can cause the RenderProcessHost to not be alive. I guess if the signin tab closes between AddScripts and LoadScripts then this could happen? Adding null check to avoid crash lgtm % the todo comment below and trying out WebUIWebViewBrowserTest.. https://codereview.chromium.org/2892353002/diff/1/extensions/browser/web_ui_u... File extensions/browser/web_ui_user_script_loader.cc (right): https://codereview.chromium.org/2892353002/diff/1/extensions/browser/web_ui_u... extensions/browser/web_ui_user_script_loader.cc:93: // |render_process_host| may no longer be alive. As we don't have a repro case, might be worth adding a TODO here to investigate as I'm not sure if there's any side effect of this happening (e.g. other assumptions might break somewhere else). WebUIWebViewBrowserTest has tests for content scripts from webui webview, you might want to see if you can write a test to repro this. If not, you can add that note to the TODO here.
The CQ bit was checked by karandeepb@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
The CQ bit was checked by karandeepb@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #3 (id:80001) has been deleted
https://codereview.chromium.org/2892353002/diff/1/extensions/browser/web_ui_u... File extensions/browser/web_ui_user_script_loader.cc (right): https://codereview.chromium.org/2892353002/diff/1/extensions/browser/web_ui_u... extensions/browser/web_ui_user_script_loader.cc:93: // |render_process_host| may no longer be alive. On 2017/05/22 23:34:53, lazyboy wrote: > As we don't have a repro case, might be worth adding a TODO here to investigate > as I'm not sure if there's any side effect of this happening (e.g. other > assumptions might break somewhere else). WebUIWebViewBrowserTest has tests for > content scripts from webui webview, you might want to see if you can write a > test to repro this. If not, you can add that note to the TODO here. Tryed adding a test in Patchset 2, but don't have a linux machine to verify it. Adding a TODO.
The CQ bit was checked by karandeepb@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lazyboy@chromium.org Link to the patchset: https://codereview.chromium.org/2892353002/#ps100001 (title: "Remove test and add TODO")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1495661831036310,
"parent_rev": "248ceac656dcee28fac9c92f2ac4cc65c97b2abd", "commit_rev":
"017234bc0a7505f2a773352e1d38090847aee163"}
Message was sent while issue was closed.
Description was changed from ========== WebUI: Fix crash while loading scripts for webview.[add/remove]ContentScripts API. WebUIUserScriptLoader is used by the DeclarativeUserScriptMaster to load scripts in webui through the declarative webview.[add/remove]ContentScripts API. As per crash reports, the cached UserScriptRenderInfo may not correspond to an alive render process host. This can happen since UserScriptLoader::LoadScripts may not be synchronous with UserScriptLoader::AddScripts. This CL tries to fix the crash by checking whether the cached UserScriptRenderInfo corresponds to a live RenderProcessHost. If it doesn't the corresponding script is not loaded. BUG=720331 ========== to ========== WebUI: Fix crash while loading scripts for webview.[add/remove]ContentScripts API. WebUIUserScriptLoader is used by the DeclarativeUserScriptMaster to load scripts in webui through the declarative webview.[add/remove]ContentScripts API. As per crash reports, the cached UserScriptRenderInfo may not correspond to an alive render process host. This can happen since UserScriptLoader::LoadScripts may not be synchronous with UserScriptLoader::AddScripts. This CL tries to fix the crash by checking whether the cached UserScriptRenderInfo corresponds to a live RenderProcessHost. If it doesn't the corresponding script is not loaded. BUG=720331 Review-Url: https://codereview.chromium.org/2892353002 Cr-Commit-Position: refs/heads/master@{#474457} Committed: https://chromium.googlesource.com/chromium/src/+/017234bc0a7505f2a773352e1d38... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:100001) as https://chromium.googlesource.com/chromium/src/+/017234bc0a7505f2a773352e1d38... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
