Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2773223002:
Fixes crash where the browser crashed when visiting WebVR pages without VR Services installed. (Closed)
DescriptionFixes crash where the browser crashed when visiting WebVR pages without VR Services installed.
- No VrDisplayImpls and subsequently VRDisplays are instantiated when querying a VRDsiplayInfo failed.
BUG=704372
Review-Url: https://codereview.chromium.org/2773223002
Cr-Commit-Position: refs/heads/master@{#460392}
Committed: https://chromium.googlesource.com/chromium/src/+/afc06442b2cfafd520fe4dbbdddf081714e53154
Patch Set 1 #
Total comments: 6
Patch Set 2 : Nit #
Total comments: 2
Patch Set 3 : Added comment for clarification #Messages
Total messages: 25 (11 generated)
tiborg@chromium.org changed reviewers: + amp@chromium.org, bajones@chromium.org
Please take a look.
billorr@chromium.org changed reviewers: + billorr@chromium.org
LGTM https://codereview.chromium.org/2773223002/diff/1/device/vr/vr_service_impl.cc File device/vr/vr_service_impl.cc (right): https://codereview.chromium.org/2773223002/diff/1/device/vr/vr_service_impl.c... device/vr/vr_service_impl.cc:74: DLOG(WARNING) NIT: Warning sounds like this is an invalid state, but this can happen for legitimate reasons.
https://codereview.chromium.org/2773223002/diff/1/device/vr/vr_service_impl.cc File device/vr/vr_service_impl.cc (right): https://codereview.chromium.org/2773223002/diff/1/device/vr/vr_service_impl.c... device/vr/vr_service_impl.cc:62: void VRServiceImpl::OnVRDisplayInfoCreated( This might have been something from the previous change, but why is onVRDisplayInfoCreated getting called at all if we don't have any VR libraries (and shouldn't have any available VR Devices)? I'm confused. Should we be changing that flow instead of adjusting the code here?
https://codereview.chromium.org/2773223002/diff/1/device/vr/vr_service_impl.cc File device/vr/vr_service_impl.cc (right): https://codereview.chromium.org/2773223002/diff/1/device/vr/vr_service_impl.c... device/vr/vr_service_impl.cc:62: void VRServiceImpl::OnVRDisplayInfoCreated( On 2017/03/24 21:51:09, amp wrote: > This might have been something from the previous change, but why is > onVRDisplayInfoCreated getting called at all if we don't have any VR libraries > (and shouldn't have any available VR Devices)? > > I'm confused. > > Should we be changing that flow instead of adjusting the code here? Long term (post Windows refactor) the device shouldn't be created if we don't have a delegate.
https://codereview.chromium.org/2773223002/diff/1/device/vr/vr_service_impl.cc File device/vr/vr_service_impl.cc (right): https://codereview.chromium.org/2773223002/diff/1/device/vr/vr_service_impl.c... device/vr/vr_service_impl.cc:62: void VRServiceImpl::OnVRDisplayInfoCreated( On 2017/03/24 22:06:54, billorr wrote: > On 2017/03/24 21:51:09, amp wrote: > > This might have been something from the previous change, but why is > > onVRDisplayInfoCreated getting called at all if we don't have any VR libraries > > (and shouldn't have any available VR Devices)? > > > > I'm confused. > > > > Should we be changing that flow instead of adjusting the code here? > > Long term (post Windows refactor) the device shouldn't be created if we don't > have a delegate. Yep, currently this is the workaround to say that the device does not exist. I agree, it would be better if we don't create this device in the first place. https://codereview.chromium.org/2773223002/diff/1/device/vr/vr_service_impl.c... device/vr/vr_service_impl.cc:74: DLOG(WARNING) On 2017/03/24 21:46:55, billorr wrote: > NIT: Warning sounds like this is an invalid state, but this can happen for > legitimate reasons. Removed the warning.
The CQ bit was checked by tiborg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from billorr@chromium.org Link to the patchset: https://codereview.chromium.org/2773223002/#ps20001 (title: "Nit")
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
No L-G-T-M from a valid reviewer yet. CQ run can only be started once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a full super star committer. Committers are members of the group "project-chromium-committers". Note that this has nothing to do with OWNERS files.
lgtm Note I'm not a committer either so you'll need to wait for bajones approval before submitting. https://codereview.chromium.org/2773223002/diff/1/device/vr/vr_service_impl.cc File device/vr/vr_service_impl.cc (right): https://codereview.chromium.org/2773223002/diff/1/device/vr/vr_service_impl.c... device/vr/vr_service_impl.cc:62: void VRServiceImpl::OnVRDisplayInfoCreated( On 2017/03/24 22:16:36, tiborg wrote: > On 2017/03/24 22:06:54, billorr wrote: > > On 2017/03/24 21:51:09, amp wrote: > > > This might have been something from the previous change, but why is > > > onVRDisplayInfoCreated getting called at all if we don't have any VR > libraries > > > (and shouldn't have any available VR Devices)? > > > > > > I'm confused. > > > > > > Should we be changing that flow instead of adjusting the code here? > > > > Long term (post Windows refactor) the device shouldn't be created if we don't > > have a delegate. > > Yep, currently this is the workaround to say that the device does not exist. I > agree, it would be better if we don't create this device in the first place. Ok, I'm fine with addressing this in a future refactor. https://codereview.chromium.org/2773223002/diff/20001/device/vr/vr_service_im... File device/vr/vr_service_impl.cc (right): https://codereview.chromium.org/2773223002/diff/20001/device/vr/vr_service_im... device/vr/vr_service_impl.cc:73: // We cannot instantiate a display with a null display info. Would be nice to call out (until we fix it) the reason that we can get null display info (because libraries don't exist etc).
https://codereview.chromium.org/2773223002/diff/20001/device/vr/vr_service_im... File device/vr/vr_service_impl.cc (right): https://codereview.chromium.org/2773223002/diff/20001/device/vr/vr_service_im... device/vr/vr_service_impl.cc:73: // We cannot instantiate a display with a null display info. On 2017/03/24 22:33:31, amp wrote: > Would be nice to call out (until we fix it) the reason that we can get null > display info (because libraries don't exist etc). Added a comment to clarify.
LGTM
The CQ bit was checked by tiborg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from amp@chromium.org, billorr@chromium.org Link to the patchset: https://codereview.chromium.org/2773223002/#ps40001 (title: "Added comment for clarification")
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: ios-simulator on master.tryserver.chromium.mac (JOB_TIMED_OUT, build hasn't started yet, builder probably lacks capacity)
The CQ bit was checked by tiborg@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": 1490798270490540,
"parent_rev": "df9bf7c32b4361a7b66a241317ca3571f58c28b3", "commit_rev":
"afc06442b2cfafd520fe4dbbdddf081714e53154"}
Message was sent while issue was closed.
Description was changed from ========== Fixes crash where the browser crashed when visiting WebVR pages without VR Services installed. - No VrDisplayImpls and subsequently VRDisplays are instantiated when querying a VRDsiplayInfo failed. BUG=704372 ========== to ========== Fixes crash where the browser crashed when visiting WebVR pages without VR Services installed. - No VrDisplayImpls and subsequently VRDisplays are instantiated when querying a VRDsiplayInfo failed. BUG=704372 Review-Url: https://codereview.chromium.org/2773223002 Cr-Commit-Position: refs/heads/master@{#460392} Committed: https://chromium.googlesource.com/chromium/src/+/afc06442b2cfafd520fe4dbbdddf... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/afc06442b2cfafd520fe4dbbdddf... |
