Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1214053004:
[d8] bounds-check before getting Shell::Worker internal field (Closed)
|
Created:
5 years, 5 months ago by caitp (gmail) Modified:
5 years, 5 months ago CC:
v8-dev Base URL:
https://chromium.googlesource.com/v8/v8.git@master Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[d8] bounds-check before getting Shell::Worker internal field
Prevents fatal error in debug builds
BUG=v8:4271, 506954
R=binji@chromium.org
LOG=N
Committed: https://crrev.com/43ce9c6f101c4224addd9a54e0c39963188dc7fa
Cr-Commit-Position: refs/heads/master@{#29524}
Committed: https://crrev.com/c9007d8f7ee634e99b3104233d7fcc2d64a18a52
Cr-Commit-Position: refs/heads/master@{#29737}
Patch Set 1 #Patch Set 2 : Also bounds-check in postMessage #Patch Set 3 : Add test #
Total comments: 8
Patch Set 4 : remove comments + IsExternal() checks #Patch Set 5 : Only test if "Worker" is a function #
Total comments: 1
Patch Set 6 : Cosmetic test changes #Messages
Total messages: 24 (7 generated)
Hey --- I dunno if anything valuable is really gained from the fatal error, given that you can always break in Isolate::Throw anyways --- and more or less figure out what happened. It might make the bug reporter happier to do it this way
caitpotter88@gmail.com changed reviewers: + binji@chromium.org
lgtm, though I'm not an owner. can you add a regression test too?
caitpotter88@gmail.com changed reviewers: + adamk@chromium.org, jochen@chromium.org
On 2015/07/06 16:52:01, binji wrote: > lgtm, though I'm not an owner. > > can you add a regression test too? Done --- +jochen/adamk
Patchset #3 (id:40001) has been deleted
https://codereview.chromium.org/1214053004/diff/60001/src/d8.cc File src/d8.cc (right): https://codereview.chromium.org/1214053004/diff/60001/src/d8.cc#newcode727 src/d8.cc:727: // Bounds-check to avoid fatal error in debug mode I'd just remove this comment: it's not a "bounds-check" in the traditional sense, it's actually checking for the existence of the internal field. https://codereview.chromium.org/1214053004/diff/60001/src/d8.cc#newcode731 src/d8.cc:731: if (this_value.IsEmpty() || !this_value->IsExternal()) { Can the second part of this conditional ever fail? I'm guessing not... https://codereview.chromium.org/1214053004/diff/60001/src/d8.cc#newcode778 src/d8.cc:778: // Bounds-check to avoid fatal error in debug mode Ditto, comment seems off. https://codereview.chromium.org/1214053004/diff/60001/src/d8.cc#newcode782 src/d8.cc:782: if (this_value.IsEmpty() || !this_value->IsExternal()) { Ditto, I don't think IsExternal can fail now https://codereview.chromium.org/1214053004/diff/60001/src/d8.cc#newcode806 src/d8.cc:806: // Bounds-check to avoid fatal error in debug mode And here... https://codereview.chromium.org/1214053004/diff/60001/src/d8.cc#newcode810 src/d8.cc:810: if (this_value.IsEmpty() || !this_value->IsExternal()) { And here
https://codereview.chromium.org/1214053004/diff/60001/src/d8.cc File src/d8.cc (right): https://codereview.chromium.org/1214053004/diff/60001/src/d8.cc#newcode727 src/d8.cc:727: // Bounds-check to avoid fatal error in debug mode On 2015/07/07 18:48:26, adamk wrote: > I'd just remove this comment: it's not a "bounds-check" in the traditional > sense, it's actually checking for the existence of the internal field. Acknowledged. https://codereview.chromium.org/1214053004/diff/60001/src/d8.cc#newcode731 src/d8.cc:731: if (this_value.IsEmpty() || !this_value->IsExternal()) { On 2015/07/07 18:48:26, adamk wrote: > Can the second part of this conditional ever fail? I'm guessing not... right now, I don't think so
The CQ bit was checked by adamk@chromium.org
lgtm
The patchset sent to the CQ was uploaded after l-g-t-m from binji@chromium.org Link to the patchset: https://codereview.chromium.org/1214053004/#ps80001 (title: "remove comments + IsExternal() checks")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1214053004/80001
Message was sent while issue was closed.
Committed patchset #4 (id:80001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/43ce9c6f101c4224addd9a54e0c39963188dc7fa Cr-Commit-Position: refs/heads/master@{#29524}
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:80001) has been created in https://codereview.chromium.org/1215333012/ by machenbach@chromium.org. The reason for reverting is: [Sheriff] Fails here: http://build.chromium.org/p/client.v8/builders/V8%20Linux%20-%20shared/builds....
Message was sent while issue was closed.
On 2015/07/07 21:16:25, Michael Achenbach wrote: > A revert of this CL (patchset #4 id:80001) has been created in > https://codereview.chromium.org/1215333012/ by mailto:machenbach@chromium.org. > > The reason for reverting is: [Sheriff] Fails here: > http://build.chromium.org/p/client.v8/builders/V8%20Linux%20-%20shared/builds.... Right, worker is only available in statically linked builds, maybe the tests don't add much then :(
https://codereview.chromium.org/1214053004/diff/100001/test/mjsunit/regress/r... File test/mjsunit/regress/regress-4271.js (right): https://codereview.chromium.org/1214053004/diff/100001/test/mjsunit/regress/r... test/mjsunit/regress/regress-4271.js:5: if (typeof Worker === 'function') test(); I've just been doing if (this.Worker) { ... }
On 2015/07/07 at 21:48:20, binji wrote: > https://codereview.chromium.org/1214053004/diff/100001/test/mjsunit/regress/r... > File test/mjsunit/regress/regress-4271.js (right): > > https://codereview.chromium.org/1214053004/diff/100001/test/mjsunit/regress/r... > test/mjsunit/regress/regress-4271.js:5: if (typeof Worker === 'function') test(); > I've just been doing > > if (this.Worker) { > ... > } lgtm. BTW, I think this (https://code.google.com/p/chromium/issues/detail?id=506954) is the same bug, can you add it to the bug line?
On 2015/07/17 21:03:19, binji wrote: > On 2015/07/07 at 21:48:20, binji wrote: > > > https://codereview.chromium.org/1214053004/diff/100001/test/mjsunit/regress/r... > > File test/mjsunit/regress/regress-4271.js (right): > > > > > https://codereview.chromium.org/1214053004/diff/100001/test/mjsunit/regress/r... > > test/mjsunit/regress/regress-4271.js:5: if (typeof Worker === 'function') > test(); > > I've just been doing > > > > if (this.Worker) { > > ... > > } > > lgtm. BTW, I think this > (https://code.google.com/p/chromium/issues/detail?id=506954) is the same bug, > can you add it to the bug line? Looks like a sec bug, I'm unable to view it. I wouldn't expect anything d8-exclusive to show up on the chromium tracker though? I'll add the bug though
The CQ bit was checked by caitpotter88@gmail.com
The patchset sent to the CQ was uploaded after l-g-t-m from adamk@chromium.org Link to the patchset: https://codereview.chromium.org/1214053004/#ps120001 (title: "Cosmetic test changes")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1214053004/120001
Message was sent while issue was closed.
Committed patchset #6 (id:120001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/c9007d8f7ee634e99b3104233d7fcc2d64a18a52 Cr-Commit-Position: refs/heads/master@{#29737} |
