Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1136463002:
Pass correct length value in WebGamepads (Closed)
DescriptionPass correct length value in WebGamepads
While building WebGamepads during enumeration, length parameter is never
reset. It is always incremented. When end user re-connects gamepad, since
this variable is always incremented, it will exceed the max number of
supported gamepads ie 4 and leads to crash in plugins.
BUG=485507
R=scottmg, raymes, bbudge
Committed: https://crrev.com/bc40fa3173a092645f2702b6b072c6b3d546cc31
Cr-Commit-Position: refs/heads/master@{#329151}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Use std::min for comparison #
Total comments: 4
Patch Set 3 : #Patch Set 4 : Fixed pending comments #
Messages
Total messages: 32 (8 generated)
PTAL https://codereview.chromium.org/1136463002/diff/1/ppapi/examples/gamepad/game... File ppapi/examples/gamepad/gamepad.cc (left): https://codereview.chromium.org/1136463002/diff/1/ppapi/examples/gamepad/game... ppapi/examples/gamepad/gamepad.cc:83: printf("NullImage\n"); Removed this since the pre submit checks were giving me a warning and not allowing me to upload the patch containing this file. https://codereview.chromium.org/1136463002/diff/1/ppapi/shared_impl/ppb_gamep... File ppapi/shared_impl/ppb_gamepad_shared.cc (right): https://codereview.chromium.org/1136463002/diff/1/ppapi/shared_impl/ppb_gamep... ppapi/shared_impl/ppb_gamepad_shared.cc:14: webkit_data.length : WebKitGamepads::kItemsLengthCap; Added this for safety.
https://codereview.chromium.org/1136463002/diff/1/ppapi/shared_impl/ppb_gamep... File ppapi/shared_impl/ppb_gamepad_shared.cc (right): https://codereview.chromium.org/1136463002/diff/1/ppapi/shared_impl/ppb_gamep... ppapi/shared_impl/ppb_gamepad_shared.cc:14: webkit_data.length : WebKitGamepads::kItemsLengthCap; On 2015/05/07 12:19:54, arajp wrote: > Added this for safety. std::min(webkit_data.length, WebKitGamepads::kItemsLengthCap) would be better here.
content/browser/gamepad/gamepad_platform_data_fetcher_win.cc lgtm
PTAL https://codereview.chromium.org/1136463002/diff/1/ppapi/shared_impl/ppb_gamep... File ppapi/shared_impl/ppb_gamepad_shared.cc (right): https://codereview.chromium.org/1136463002/diff/1/ppapi/shared_impl/ppb_gamep... ppapi/shared_impl/ppb_gamepad_shared.cc:14: webkit_data.length : WebKitGamepads::kItemsLengthCap; On 2015/05/07 13:58:57, bbudge wrote: > On 2015/05/07 12:19:54, arajp wrote: > > Added this for safety. > > std::min(webkit_data.length, WebKitGamepads::kItemsLengthCap) would be better > here. Done.
The CQ bit was checked by arajp@nvidia.com to run a CQ dry run
The patchset sent to the CQ was uploaded after l-g-t-m from scottmg@chromium.org Link to the patchset: https://codereview.chromium.org/1136463002/#ps20001 (title: "Use std::min for comparison")
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1136463002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_gn_dbg on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
On 2015/05/08 06:18:57, I haz the power (commit-bot) wrote: > Dry run: Try jobs failed on following builders: > linux_chromium_gn_dbg on tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) I am not sure why this is failing with error: ../../ppapi/shared_impl/ppb_gamepad_shared.cc:14: error: undefined reference to 'ppapi::WebKitGamepads::kItemsLengthCap' Can someone help me fix this problem? I tried changing WebKitGamepads struct to make it visible using PPAPI_SHARED_EXPORT but that did not work.
On 2015/05/08 07:39:48, arajp%nvidia.com wrote: > On 2015/05/08 06:18:57, I haz the power (commit-bot) wrote: > > Dry run: Try jobs failed on following builders: > > linux_chromium_gn_dbg on tryserver.chromium.linux (JOB_FAILED, > > > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) > > I am not sure why this is failing with error: > ../../ppapi/shared_impl/ppb_gamepad_shared.cc:14: error: undefined reference to > 'ppapi::WebKitGamepads::kItemsLengthCap' > Can someone help me fix this problem? I tried changing WebKitGamepads struct to > make it visible using PPAPI_SHARED_EXPORT but that did not work. You have to pull the definition of the constant out of the struct where it is declared (and currently also defined). http://stackoverflow.com/questions/5508182/static-const-int-causes-linking-er...
https://codereview.chromium.org/1136463002/diff/20001/ppapi/shared_impl/ppb_g... File ppapi/shared_impl/ppb_gamepad_shared.cc (right): https://codereview.chromium.org/1136463002/diff/20001/ppapi/shared_impl/ppb_g... ppapi/shared_impl/ppb_gamepad_shared.cc:16: (const size_t)webkit_data.length); Use C++ style cast. Also, you could avoid a cast by declaring length to be uint32_t here.
The CQ bit was checked by arajp@nvidia.com to run a CQ dry run
The patchset sent to the CQ was uploaded after l-g-t-m from scottmg@chromium.org Link to the patchset: https://codereview.chromium.org/1136463002/#ps40001 (title: " ")
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1136463002/40001
The CQ bit was unchecked by arajp@nvidia.com
https://codereview.chromium.org/1136463002/diff/20001/ppapi/shared_impl/ppb_g... File ppapi/shared_impl/ppb_gamepad_shared.cc (right): https://codereview.chromium.org/1136463002/diff/20001/ppapi/shared_impl/ppb_g... ppapi/shared_impl/ppb_gamepad_shared.cc:16: (const size_t)webkit_data.length); On 2015/05/08 17:31:52, bbudge wrote: > Use C++ style cast. Also, you could avoid a cast by declaring length to be > uint32_t here. Changed to C++ style cast. Changing length to uint32_t required a type cast at std::min, else Windows build was complaining with: warning C4267: 'initializing' : conversion from 'size_t' to 'uint32_t', possible loss of data So I had to typecast either at min or at the line below. I left it at the line below.
Please see if this fix can be pulled in to any earlier release of Chrome, not waiting till v 44. This is a serious issue since gamepad reconnection of 4 times is for the entire browser session and not for one Plugin session. Users usually have one Browser session open for quite a long time. They dont restart Browser and the 4 gamepad reconnections scenario can happen quite easily.
LGTM w/nit and suggestion https://codereview.chromium.org/1136463002/diff/20001/ppapi/shared_impl/ppb_g... File ppapi/shared_impl/ppb_gamepad_shared.cc (right): https://codereview.chromium.org/1136463002/diff/20001/ppapi/shared_impl/ppb_g... ppapi/shared_impl/ppb_gamepad_shared.cc:16: (const size_t)webkit_data.length); On 2015/05/11 10:40:40, arajp wrote: > On 2015/05/08 17:31:52, bbudge wrote: > > Use C++ style cast. Also, you could avoid a cast by declaring length to be > > uint32_t here. > > Changed to C++ style cast. Changing length to uint32_t required a type cast at > std::min, else Windows build was complaining with: > warning C4267: 'initializing' : conversion from 'size_t' to 'uint32_t', possible > loss of data > So I had to typecast either at min or at the line below. I left it at the line > below. Would using 'unsigned' to match WebKitGamepads help? Otherwise, it's not worth too much fussing; Nit: indent is off by 1 space.
Submitting a new patch in a moment. https://codereview.chromium.org/1136463002/diff/20001/ppapi/shared_impl/ppb_g... File ppapi/shared_impl/ppb_gamepad_shared.cc (right): https://codereview.chromium.org/1136463002/diff/20001/ppapi/shared_impl/ppb_g... ppapi/shared_impl/ppb_gamepad_shared.cc:16: (const size_t)webkit_data.length); On 2015/05/11 11:21:29, bbudge (OOO week of 5-11) wrote: > On 2015/05/11 10:40:40, arajp wrote: > > On 2015/05/08 17:31:52, bbudge wrote: > > > Use C++ style cast. Also, you could avoid a cast by declaring length to be > > > uint32_t here. > > > > Changed to C++ style cast. Changing length to uint32_t required a type cast at > > std::min, else Windows build was complaining with: > > warning C4267: 'initializing' : conversion from 'size_t' to 'uint32_t', > possible > > loss of data > > So I had to typecast either at min or at the line below. I left it at the line > > below. > > Would using 'unsigned' to match WebKitGamepads help? Otherwise, it's not worth > too much fussing; > Nit: indent is off by 1 space. Done.
Done. Can you also recommend this fix to be pulled in to Chrome 42/43?
On 2015/05/11 12:19:39, arajp wrote: > Done. > Can you also recommend this fix to be pulled in to Chrome 42/43? You can do that on the bug, once you land this on trunk.
The CQ bit was checked by arajp@nvidia.com
The patchset sent to the CQ was uploaded after l-g-t-m from bbudge@chromium.org, scottmg@chromium.org Link to the patchset: https://codereview.chromium.org/1136463002/#ps60001 (title: "Fixed pending comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1136463002/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/bc40fa3173a092645f2702b6b072c6b3d546cc31 Cr-Commit-Position: refs/heads/master@{#329151}
Message was sent while issue was closed.
@bbudge I requested for integration to an earlier branch on the bug but there was no response. Can you help? Users have no way to figure out what to do in case their plugin crashes when gamepad is connected. It leads to a very poor user experience.
Message was sent while issue was closed.
On 2015/05/18 04:48:55, arajp wrote: > @bbudge > I requested for integration to an earlier branch on the bug but there was no > response. Can you help? > > Users have no way to figure out what to do in case their plugin crashes when > gamepad is connected. It leads to a very poor user experience.
Message was sent while issue was closed.
On 2015/05/18 17:48:48, bbudge (OOO week of 5-11) wrote: > On 2015/05/18 04:48:55, arajp wrote: > > @bbudge > > I requested for integration to an earlier branch on the bug but there was no > > response. Can you help? > > > > Users have no way to figure out what to do in case their plugin crashes when > > gamepad is connected. It leads to a very poor user experience. Now that the change has landed on trunk and been in a Canary, I'll request the merge. Did this make the M44 branch cut?
Message was sent while issue was closed.
On 2015/05/18 17:50:07, bbudge (OOO week of 5-11) wrote: > On 2015/05/18 17:48:48, bbudge (OOO week of 5-11) wrote: > > On 2015/05/18 04:48:55, arajp wrote: > > > @bbudge > > > I requested for integration to an earlier branch on the bug but there was no > > > response. Can you help? > > > > > > Users have no way to figure out what to do in case their plugin crashes when > > > gamepad is connected. It leads to a very poor user experience. > > Now that the change has landed on trunk and been in a Canary, I'll request the > merge. Did this make the M44 branch cut? Actually, I can't determine if this made M44. So I'll wait until that is clearer. We merge back to branches one at a time, most recent first.
Message was sent while issue was closed.
On 2015/05/18 17:54:49, bbudge Munich week of 5-18 wrote: > On 2015/05/18 17:50:07, bbudge (OOO week of 5-11) wrote: > > On 2015/05/18 17:48:48, bbudge (OOO week of 5-11) wrote: > > > On 2015/05/18 04:48:55, arajp wrote: > > > > @bbudge > > > > I requested for integration to an earlier branch on the bug but there was > no > > > > response. Can you help? > > > > > > > > Users have no way to figure out what to do in case their plugin crashes > when > > > > gamepad is connected. It leads to a very poor user experience. > > > > Now that the change has landed on trunk and been in a Canary, I'll request the > > merge. Did this make the M44 branch cut? > > Actually, I can't determine if this made M44. So I'll wait until that is > clearer. We merge back to branches one at a time, most recent first. This is in M44. Requested merge to M43. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
