Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2146323003:
Made CrashingModules.plist non-updatable. (Closed)
|
Created:
4 years, 5 months ago by pkl (ping after 24h if needed) Modified:
4 years, 5 months ago CC:
chromium-reviews, sdefresne+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMade CrashingModules.plist non-updatable.
The server-update capability was never used in practice. The existence
poses a potential risk of google3 misconfiguration causing all instances
of Chrome iOS to crash on start up.
https://chromereviews.googleplex.com/466337014/ depends on this CL.
BUG=484694
Committed: https://crrev.com/44bab48d608f3de6ebdb409a0ba5375221dd049a
Cr-Commit-Position: refs/heads/master@{#405871}
Patch Set 1 #
Total comments: 2
Messages
Total messages: 22 (9 generated)
Description was changed from ========== Made CrashingModules.plist non-updatable. The server-update capability was never used in practice. The existence poses a potential risk of google3 misconfiguration causing all instances of Chrome iOS to crash on start up. BUG=484694 ========== to ========== Made CrashingModules.plist non-updatable. The server-update capability was never used in practice. The existence poses a potential risk of google3 misconfiguration causing all instances of Chrome iOS to crash on start up. https://chromereviews.googleplex.com/466337014/ depends on this CL. BUG=484694 ==========
pkl@chromium.org changed reviewers: + justincohen@google.com
justincohen@chromium.org changed reviewers: + justincohen@chromium.org
lgtm
The CQ bit was checked by pkl@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 unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
pkl@chromium.org changed reviewers: + rohitrao@chromium.org
+rohitrao for OWNERS
lgtm https://codereview.chromium.org/2146323003/diff/1/ios/chrome/app/safe_mode_cr... File ios/chrome/app/safe_mode_crashing_modules_config.mm (right): https://codereview.chromium.org/2146323003/diff/1/ios/chrome/app/safe_mode_cr... ios/chrome/app/safe_mode_crashing_modules_config.mm:44: NSDictionary* modules = base::mac::ObjCCastStrict<NSDictionary>( Does this method also get called during startup? What happens if the cast fails? (What does modules[modulePath] do if modules is nil? Should we be more defensive here?)
Not lgtm (Did not mean to lg this yet, wanted to make sure the new code can't crash at startup either.)
Answered rohitrao's questions. https://codereview.chromium.org/2146323003/diff/1/ios/chrome/app/safe_mode_cr... File ios/chrome/app/safe_mode_crashing_modules_config.mm (right): https://codereview.chromium.org/2146323003/diff/1/ios/chrome/app/safe_mode_cr... ios/chrome/app/safe_mode_crashing_modules_config.mm:44: NSDictionary* modules = base::mac::ObjCCastStrict<NSDictionary>( On 2016/07/15 17:23:19, rohitrao wrote: > Does this method also get called during startup? What happens if the cast > fails? (What does modules[modulePath] do if modules is nil? Should we be more > defensive here?) This method is currently called from Safe Mode View Controller. If modules in modules[modulePath] is nil, nil is returned. ObjCCastStrict DCHECKs and returns nil if object is not a compatible type. Nil will be returned from this method. The behavior of using ObjCCastStrict and ObjCCast is equivalent to the replaced version, just less verbose.
LGTM The paranoid part of me would maybe prefer explicit nil checks rather than relying on messaging nil, but that's not a big deal.
On 2016/07/15 20:38:14, rohitrao wrote: > LGTM > > The paranoid part of me would maybe prefer explicit nil checks rather than > relying on messaging nil, but that's not a big deal. If it brings you any comfort, ObjC style guide explicitly said "Use nil pointer checks for logic flow" which I think applies in this case.
The CQ bit was checked by pkl@chromium.org
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 ========== Made CrashingModules.plist non-updatable. The server-update capability was never used in practice. The existence poses a potential risk of google3 misconfiguration causing all instances of Chrome iOS to crash on start up. https://chromereviews.googleplex.com/466337014/ depends on this CL. BUG=484694 ========== to ========== Made CrashingModules.plist non-updatable. The server-update capability was never used in practice. The existence poses a potential risk of google3 misconfiguration causing all instances of Chrome iOS to crash on start up. https://chromereviews.googleplex.com/466337014/ depends on this CL. BUG=484694 ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== Made CrashingModules.plist non-updatable. The server-update capability was never used in practice. The existence poses a potential risk of google3 misconfiguration causing all instances of Chrome iOS to crash on start up. https://chromereviews.googleplex.com/466337014/ depends on this CL. BUG=484694 ========== to ========== Made CrashingModules.plist non-updatable. The server-update capability was never used in practice. The existence poses a potential risk of google3 misconfiguration causing all instances of Chrome iOS to crash on start up. https://chromereviews.googleplex.com/466337014/ depends on this CL. BUG=484694 Committed: https://crrev.com/44bab48d608f3de6ebdb409a0ba5375221dd049a Cr-Commit-Position: refs/heads/master@{#405871} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/44bab48d608f3de6ebdb409a0ba5375221dd049a Cr-Commit-Position: refs/heads/master@{#405871} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
