|
|
DescriptionSpeculative fix for crasher.
I speculate that WebAXObject::IsDetached() may not be as reliable
as Document::IsDetached().
Also, add a SECURITY_CHECK in the place where I suspect the bad
access is happening.
BUG=722801
R=dmazzoni@chromium.org,skobes@chromium.org
Review-Url: https://codereview.chromium.org/2892283002
Cr-Commit-Position: refs/heads/master@{#473448}
Committed: https://chromium.googlesource.com/chromium/src/+/fc9849ac4fd3411fe46254ab10405d7ceae0a5b6
Patch Set 1 #
Total comments: 6
Patch Set 2 : s/SECURITY_CHECK/CHECK/ #Patch Set 3 : Add TODO's #
Messages
Total messages: 23 (9 generated)
lgtm
Description was changed from ========== Speculative fix for crasher. I speculate that WebAXObject::IsDetached() may not be as reliable as Document::IsDetached(). Also, add a SECURITY_CHECK in the place where I suspect the bad access is happening. BUG=722801 R=dmazzoni@chromium.org,skobes@chromium.org ========== to ========== Speculative fix for crasher. I speculate that WebAXObject::IsDetached() may not be as reliable as Document::IsDetached(). Also, add a SECURITY_CHECK in the place where I suspect the bad access is happening. BUG=722801 R=dmazzoni@chromium.org,skobes@chromium.org ==========
szager@google.com changed reviewers: + pdr@chromium.org
szager@google.com changed reviewers: + szager@google.com
+pdr for web/OWNERS
https://codereview.chromium.org/2892283002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/frame/ResizeViewportAnchor.cpp (right): https://codereview.chromium.org/2892283002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/ResizeViewportAnchor.cpp:18: SECURITY_CHECK(frame_view); Is this actually related to security? If not, what about just CHECK?
https://codereview.chromium.org/2892283002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/frame/ResizeViewportAnchor.cpp (right): https://codereview.chromium.org/2892283002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/ResizeViewportAnchor.cpp:18: SECURITY_CHECK(frame_view); On 2017/05/19 22:47:50, pdr. wrote: > Is this actually related to security? If not, what about just CHECK? Done.
drive-bys https://codereview.chromium.org/2892283002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/frame/ResizeViewportAnchor.cpp (right): https://codereview.chromium.org/2892283002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/ResizeViewportAnchor.cpp:18: SECURITY_CHECK(frame_view); On 2017/05/19 22:47:50, pdr. wrote: > Is this actually related to security? If not, what about just CHECK? If this absolutely must be a CHECK(), it should be temporary and should come with a TODO to remove: https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... (This CHECK normally doesn't add any value; it's possible to figure out that frame_view is null from the crash dumps) https://codereview.chromium.org/2892283002/diff/1/third_party/WebKit/Source/w... File third_party/WebKit/Source/web/WebAXObject.cpp (right): https://codereview.chromium.org/2892283002/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/web/WebAXObject.cpp:163: if (!document || !document->View() || document->IsDetached()) Can we add a TODO for accessibility to see if we can sync the meaning of IsDetached() here? It's confusing to have this.
https://codereview.chromium.org/2892283002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/frame/ResizeViewportAnchor.cpp (right): https://codereview.chromium.org/2892283002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/ResizeViewportAnchor.cpp:18: SECURITY_CHECK(frame_view); On 2017/05/19 22:55:08, dcheng (in AEST) wrote: > On 2017/05/19 22:47:50, pdr. wrote: > > Is this actually related to security? If not, what about just CHECK? > > If this absolutely must be a CHECK(), it should be temporary and should come > with a TODO to remove: > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... > > (This CHECK normally doesn't add any value; it's possible to figure out that > frame_view is null from the crash dumps) Done. https://codereview.chromium.org/2892283002/diff/1/third_party/WebKit/Source/w... File third_party/WebKit/Source/web/WebAXObject.cpp (right): https://codereview.chromium.org/2892283002/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/web/WebAXObject.cpp:163: if (!document || !document->View() || document->IsDetached()) On 2017/05/19 22:55:08, dcheng (in AEST) wrote: > Can we add a TODO for accessibility to see if we can sync the meaning of > IsDetached() here? It's confusing to have this. Done.
I just loaded the minidump in a debugger and frame_view is definitely null. Do we still need the check?
On 2017/05/19 23:14:03, pdr. wrote: > I just loaded the minidump in a debugger and frame_view is definitely null. Do > we still need the check? Is better or worse to hit a CHECK as opposed to using a null pointer?
On 2017/05/19 at 23:24:33, szager wrote: > On 2017/05/19 23:14:03, pdr. wrote: > > I just loaded the minidump in a debugger and frame_view is definitely null. Do > > we still need the check? > > Is better or worse to hit a CHECK as opposed to using a null pointer? I think they are equivalent in this case. The CHECK just means you have to land a second patch sometime in the future to remove it. I don't think it's a big deal either way. LGTM for either.
The CQ bit was checked by szager@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from skobes@chromium.org Link to the patchset: https://codereview.chromium.org/2892283002/#ps40001 (title: "Add TODO's")
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 szager@chromium.org
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": 40001, "attempt_start_ts": 1495309692089900, "parent_rev": "252214d2c6d64400632711b960099c3ffb2333c2", "commit_rev": "fc9849ac4fd3411fe46254ab10405d7ceae0a5b6"}
Message was sent while issue was closed.
Description was changed from ========== Speculative fix for crasher. I speculate that WebAXObject::IsDetached() may not be as reliable as Document::IsDetached(). Also, add a SECURITY_CHECK in the place where I suspect the bad access is happening. BUG=722801 R=dmazzoni@chromium.org,skobes@chromium.org ========== to ========== Speculative fix for crasher. I speculate that WebAXObject::IsDetached() may not be as reliable as Document::IsDetached(). Also, add a SECURITY_CHECK in the place where I suspect the bad access is happening. BUG=722801 R=dmazzoni@chromium.org,skobes@chromium.org Review-Url: https://codereview.chromium.org/2892283002 Cr-Commit-Position: refs/heads/master@{#473448} Committed: https://chromium.googlesource.com/chromium/src/+/fc9849ac4fd3411fe46254ab1040... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/fc9849ac4fd3411fe46254ab1040... |