Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2299943002:
Record the PDF and top level URL when the PDF plugin crashes. (Closed)
|
Created:
4 years, 3 months ago by Lei Zhang Modified:
4 years, 3 months ago CC:
chromium-reviews, extensions-reviews_chromium.org, yusukes+watch_chromium.org, tzik, binji+watch_chromium.org, piman+watch_chromium.org, teravest+watch_chromium.org, arv+watch_chromium.org, bradnelson+warch_chromium.org, chromium-apps-reviews_chromium.org, ihf+watch_chromium.org, dsinclair Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRecord the PDF and top level URL when the PDF plugin crashes.
BUG=638716
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/383ff955ecb5eddb6a91eae873f617f814426cc4
Cr-Commit-Position: refs/heads/master@{#417214}
Patch Set 1 #
Total comments: 17
Patch Set 2 : Address comments, fix JS #
Total comments: 1
Patch Set 3 : Remove logging #
Total comments: 8
Patch Set 4 : rebase #Patch Set 5 : promises in // #Patch Set 6 : fix typo #
Messages
Total messages: 45 (29 generated)
Description was changed from ========== Record the PDF and embedder URL when the PDF plugin crashes. BUG=638716 ========== to ========== Record the PDF and embedder URL when the PDF plugin crashes. BUG=638716 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by thestig@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.
thestig@chromium.org changed reviewers: + raymes@chromium.org
Oh, I should mention, since we have the 1 PDF plugin process for all the PDFs architecture, this records the URLS for last PDF opened, not the one that actually crashed.
dsinclair: ^ FYI regarding the caveat with this CL. Flash has the same problem.
https://codereview.chromium.org/2299943002/diff/1/chrome/browser/resources/pd... File chrome/browser/resources/pdf/browser_api.js (right): https://codereview.chromium.org/2299943002/diff/1/chrome/browser/resources/pd... chrome/browser/resources/pdf/browser_api.js:155: let manageZoom = !streamInfo.embedded && streamInfo.tabId != -1; It does feel a bit weird that we pass in whether the the PDF is embedded in the streamInfo but we don't pass the URL of the embedder (we could even consider merging those two parameters - empty implies top level frame). Perhaps we should just do that? I think it would happen here: https://cs.chromium.org/chromium/src/chrome/browser/renderer_host/chrome_reso... This would reduce the indirection here (this file is already full of indirection as I'm sure you've noticed!). Saying that if it's a lot more complicated to do that, this seems ok. I'll let you take a look and decide. https://codereview.chromium.org/2299943002/diff/1/chrome/browser/resources/pd... chrome/browser/resources/pdf/browser_api.js:162: } Hmm this doesn't look quite right to me. resolve() will get called here and then again after we setZoomSettings, is that right? I think we may want to chain another .then() before we get to this function which adds the new parameter in. I might be missing something though! https://codereview.chromium.org/2299943002/diff/1/chrome/browser/resources/pd... File chrome/browser/resources/pdf/pdf.js (right): https://codereview.chromium.org/2299943002/diff/1/chrome/browser/resources/pd... chrome/browser/resources/pdf/pdf.js:206: this.plugin_.setAttribute('embedder-url', I think top-level-url may be more accurate here (there may be several layers of embedder, but we will only record the top level one. https://codereview.chromium.org/2299943002/diff/1/chrome/browser/resources/pd... chrome/browser/resources/pdf/pdf.js:209: this.plugin_.setAttribute('full-frame', ''); optional: We could merge this with top-level-url and define empty as full-frame. It would save parsing an additional param (though we would still probably want to record a URL in the crash report). I'll leave that up to you. https://codereview.chromium.org/2299943002/diff/1/ppapi/c/private/ppb_pdf.h File ppapi/c/private/ppb_pdf.h (right): https://codereview.chromium.org/2299943002/diff/1/ppapi/c/private/ppb_pdf.h#n... ppapi/c/private/ppb_pdf.h:164: const char* embedder_url); nit: top_level_url https://codereview.chromium.org/2299943002/diff/1/ppapi/cpp/private/pdf.cc File ppapi/cpp/private/pdf.cc (right): https://codereview.chromium.org/2299943002/diff/1/ppapi/cpp/private/pdf.cc#ne... ppapi/cpp/private/pdf.cc:189: const char* embedder_url) { nit: top_level_url https://codereview.chromium.org/2299943002/diff/1/ppapi/proxy/pdf_resource.h File ppapi/proxy/pdf_resource.h (right): https://codereview.chromium.org/2299943002/diff/1/ppapi/proxy/pdf_resource.h#... ppapi/proxy/pdf_resource.h:65: void SetCrashData(const char* pdf_url, const char* embedder_url) override; nit: top_level_url https://codereview.chromium.org/2299943002/diff/1/ppapi/thunk/ppb_pdf_api.h File ppapi/thunk/ppb_pdf_api.h (right): https://codereview.chromium.org/2299943002/diff/1/ppapi/thunk/ppb_pdf_api.h#n... ppapi/thunk/ppb_pdf_api.h:43: virtual void SetCrashData(const char* pdf_url, const char* embedder_url) = 0; nit: top_level_url https://codereview.chromium.org/2299943002/diff/1/ppapi/thunk/ppb_pdf_thunk.cc File ppapi/thunk/ppb_pdf_thunk.cc (right): https://codereview.chromium.org/2299943002/diff/1/ppapi/thunk/ppb_pdf_thunk.c... ppapi/thunk/ppb_pdf_thunk.cc:165: const char* embedder_url) { nit: top_level_url
Description was changed from ========== Record the PDF and embedder URL when the PDF plugin crashes. BUG=638716 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Record the PDF and top level URL when the PDF plugin crashes. BUG=638716 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by thestig@chromium.org to run a CQ dry run
https://codereview.chromium.org/2299943002/diff/1/chrome/browser/resources/pd... File chrome/browser/resources/pdf/browser_api.js (right): https://codereview.chromium.org/2299943002/diff/1/chrome/browser/resources/pd... chrome/browser/resources/pdf/browser_api.js:162: } On 2016/09/05 04:11:47, raymes wrote: > Hmm this doesn't look quite right to me. resolve() will get called here and then > again after we setZoomSettings, is that right? I think we may want to chain > another .then() before we get to this function which adds the new parameter in. > > I might be missing something though! Uhh, no idea what I'm doing exactly. Let's take another stab at this... https://codereview.chromium.org/2299943002/diff/1/chrome/browser/resources/pd... File chrome/browser/resources/pdf/pdf.js (right): https://codereview.chromium.org/2299943002/diff/1/chrome/browser/resources/pd... chrome/browser/resources/pdf/pdf.js:206: this.plugin_.setAttribute('embedder-url', On 2016/09/05 04:11:47, raymes wrote: > I think top-level-url may be more accurate here (there may be several layers of > embedder, but we will only record the top level one. Done in all the places. https://codereview.chromium.org/2299943002/diff/1/chrome/browser/resources/pd... chrome/browser/resources/pdf/pdf.js:209: this.plugin_.setAttribute('full-frame', ''); On 2016/09/05 04:11:47, raymes wrote: > optional: We could merge this with top-level-url and define empty as full-frame. > It would save parsing an additional param (though we would still probably want > to record a URL in the crash report). I'll leave that up to you. Can we ever be embedded, but have an empty top-level URL somehow? I'm not 100% sure, so I've left the two separate. https://codereview.chromium.org/2299943002/diff/1/ppapi/c/private/ppb_pdf.h File ppapi/c/private/ppb_pdf.h (right): https://codereview.chromium.org/2299943002/diff/1/ppapi/c/private/ppb_pdf.h#n... ppapi/c/private/ppb_pdf.h:164: const char* embedder_url); On 2016/09/05 04:11:47, raymes wrote: > nit: top_level_url Done. https://codereview.chromium.org/2299943002/diff/1/ppapi/cpp/private/pdf.cc File ppapi/cpp/private/pdf.cc (right): https://codereview.chromium.org/2299943002/diff/1/ppapi/cpp/private/pdf.cc#ne... ppapi/cpp/private/pdf.cc:189: const char* embedder_url) { On 2016/09/05 04:11:47, raymes wrote: > nit: top_level_url Done. https://codereview.chromium.org/2299943002/diff/1/ppapi/proxy/pdf_resource.h File ppapi/proxy/pdf_resource.h (right): https://codereview.chromium.org/2299943002/diff/1/ppapi/proxy/pdf_resource.h#... ppapi/proxy/pdf_resource.h:65: void SetCrashData(const char* pdf_url, const char* embedder_url) override; On 2016/09/05 04:11:47, raymes wrote: > nit: top_level_url Done. https://codereview.chromium.org/2299943002/diff/1/ppapi/thunk/ppb_pdf_api.h File ppapi/thunk/ppb_pdf_api.h (right): https://codereview.chromium.org/2299943002/diff/1/ppapi/thunk/ppb_pdf_api.h#n... ppapi/thunk/ppb_pdf_api.h:43: virtual void SetCrashData(const char* pdf_url, const char* embedder_url) = 0; On 2016/09/05 04:11:47, raymes wrote: > nit: top_level_url Done. https://codereview.chromium.org/2299943002/diff/1/ppapi/thunk/ppb_pdf_thunk.cc File ppapi/thunk/ppb_pdf_thunk.cc (right): https://codereview.chromium.org/2299943002/diff/1/ppapi/thunk/ppb_pdf_thunk.c... ppapi/thunk/ppb_pdf_thunk.cc:165: const char* embedder_url) { On 2016/09/05 04:11:47, raymes wrote: > nit: top_level_url Done.
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: Try jobs failed on following builders: closure_compilation on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/closure_compila...)
The CQ bit was checked by thestig@chromium.org to run a CQ dry run
https://codereview.chromium.org/2299943002/diff/20001/pdf/out_of_process_inst... File pdf/out_of_process_instance.cc (right): https://codereview.chromium.org/2299943002/diff/20001/pdf/out_of_process_inst... pdf/out_of_process_instance.cc:382: LOG(ERROR) << "original_url " << original_url; Whoops!
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: Try jobs failed on following builders: closure_compilation on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/closure_compila...)
raymes@chromium.org changed reviewers: + sammc@chromium.org
lgtm with a comment for sammc https://codereview.chromium.org/2299943002/diff/40001/chrome/browser/resource... File chrome/browser/resources/pdf/browser_api.js (right): https://codereview.chromium.org/2299943002/diff/40001/chrome/browser/resource... chrome/browser/resources/pdf/browser_api.js:157: resolve(false); nit: Maybe just resolve() or resolve(null) since the expected type is an object? https://codereview.chromium.org/2299943002/diff/40001/chrome/browser/resource... chrome/browser/resources/pdf/browser_api.js:172: }).then(function() { return BrowserApi.create(streamInfo, manageZoom); }); This looks good to me, but I'm also not great with promises. +sammc to have an additional look https://codereview.chromium.org/2299943002/diff/40001/pdf/out_of_process_inst... File pdf/out_of_process_instance.cc (right): https://codereview.chromium.org/2299943002/diff/40001/pdf/out_of_process_inst... pdf/out_of_process_instance.cc:354: else if (!full_ && strcmp(argn[i], "top-level-url") == 0) I'd be ok to set this even if full_ is true. What were your thoughts?
https://codereview.chromium.org/2299943002/diff/40001/chrome/browser/resource... File chrome/browser/resources/pdf/browser_api.js (right): https://codereview.chromium.org/2299943002/diff/40001/chrome/browser/resource... chrome/browser/resources/pdf/browser_api.js:155: return new Promise(function(resolve, reject) { I'd prefer doing both API calls in parallel. Something like let promises = []; if (streamInfo.tabId != -1) { promises.push(new Promise(resolve => { chrome.tabs.get(..., resolve); }).then(tab => { if (tab) streamInfo.tabUrl = tab.url; }); if (!streamInfo.embedded) { promises.push(new Promise(resolve => { chrome.tabs.setZoomSettings(..., resolve); })); } } return Promise.all(promises);
https://codereview.chromium.org/2299943002/diff/40001/chrome/browser/resource... File chrome/browser/resources/pdf/browser_api.js (right): https://codereview.chromium.org/2299943002/diff/40001/chrome/browser/resource... chrome/browser/resources/pdf/browser_api.js:155: return new Promise(function(resolve, reject) { On 2016/09/07 07:53:02, Sam McNally wrote: > I'd prefer doing both API calls in parallel. Something like > let promises = []; > if (streamInfo.tabId != -1) { > promises.push(new Promise(resolve => { > chrome.tabs.get(..., resolve); > }).then(tab => { > if (tab) > streamInfo.tabUrl = tab.url; > }); > if (!streamInfo.embedded) { > promises.push(new Promise(resolve => { > chrome.tabs.setZoomSettings(..., resolve); > })); > } > } > return Promise.all(promises); Done. Hope I got it right. A JS presubmit check does not like => syntax. https://codereview.chromium.org/2299943002/diff/40001/chrome/browser/resource... chrome/browser/resources/pdf/browser_api.js:157: resolve(false); On 2016/09/07 06:10:14, raymes wrote: > nit: Maybe just resolve() or resolve(null) since the expected type is an object? Done. https://codereview.chromium.org/2299943002/diff/40001/chrome/browser/resource... chrome/browser/resources/pdf/browser_api.js:172: }).then(function() { return BrowserApi.create(streamInfo, manageZoom); }); On 2016/09/07 06:10:14, raymes wrote: > This looks good to me, but I'm also not great with promises. +sammc to have an > additional look Acknowledged. https://codereview.chromium.org/2299943002/diff/40001/pdf/out_of_process_inst... File pdf/out_of_process_instance.cc (right): https://codereview.chromium.org/2299943002/diff/40001/pdf/out_of_process_inst... pdf/out_of_process_instance.cc:354: else if (!full_ && strcmp(argn[i], "top-level-url") == 0) On 2016/09/07 06:10:14, raymes wrote: > I'd be ok to set this even if full_ is true. What were your thoughts? Sure. Not a big deal either way.
lgtm
The CQ bit was checked by thestig@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from raymes@chromium.org Link to the patchset: https://codereview.chromium.org/2299943002/#ps80001 (title: "promises in //")
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
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by thestig@chromium.org
The CQ bit was unchecked by thestig@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by thestig@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: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by thestig@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sammc@chromium.org, raymes@chromium.org Link to the patchset: https://codereview.chromium.org/2299943002/#ps100001 (title: "fix typo")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Record the PDF and top level URL when the PDF plugin crashes. BUG=638716 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Record the PDF and top level URL when the PDF plugin crashes. BUG=638716 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Record the PDF and top level URL when the PDF plugin crashes. BUG=638716 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Record the PDF and top level URL when the PDF plugin crashes. BUG=638716 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/383ff955ecb5eddb6a91eae873f617f814426cc4 Cr-Commit-Position: refs/heads/master@{#417214} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/383ff955ecb5eddb6a91eae873f617f814426cc4 Cr-Commit-Position: refs/heads/master@{#417214} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
