Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2548653004:
win64: fix lookup of RtlAddFunctionTable() helper for v8 (Closed)
Descriptionwin64: fix lookup of RtlAddFunctionTable() helper for v8
This helper function moved from chrome.exe to chrome_elf. The failure to
look up is unfortunately silent because the same code is used in test
binaries. As a result, RtlAddFunctionTable() wasn't called, so any
crashes with v8 jit on the stack on Win x64 caused program termination
without giving our crash handler a chance to catch it.
R=thakis@chromium.org, jochen@chromium.org
BUG=670466, 656800, 604923, v8:3598
Committed: https://crrev.com/56c90a89889693bd6e1a7d7bb4b8f49c69bab2fd
Cr-Commit-Position: refs/heads/master@{#436041}
Patch Set 1 #
Messages
Total messages: 15 (7 generated)
Description was changed from ========== win64: fix lookup of RtlAddFunctionTable() helper for v8 This helper function moved from chrome.exe to chrome_elf. The failure to look up is unfortunately silent because the same code is used it test binaries. As a result, RtlAddFunctionTable() wasn't called, so any crashes with v8 jit on the stack on Win x64 caused program termination without giving our crash handler a chance to catch it. R=thakis@chromium.org BUG=670466,656800,604923,v8:3598 ========== to ========== win64: fix lookup of RtlAddFunctionTable() helper for v8 This helper function moved from chrome.exe to chrome_elf. The failure to look up is unfortunately silent because the same code is used it test binaries. As a result, RtlAddFunctionTable() wasn't called, so any crashes with v8 jit on the stack on Win x64 caused program termination without giving our crash handler a chance to catch it. R=thakis@chromium.org, jochen@chromium.org BUG=670466,656800,604923,v8:3598 ==========
scottmg@chromium.org changed reviewers: + jochen@chromium.org
Nico: Jochen is the right OWNERS to review this, but as it's already weekend there, would you mind reviewing? (This caused the largest crasher to be invisible on the last Win Canary x64, so I'd like to fix it sooner than Monday if we can.)
Description was changed from ========== win64: fix lookup of RtlAddFunctionTable() helper for v8 This helper function moved from chrome.exe to chrome_elf. The failure to look up is unfortunately silent because the same code is used it test binaries. As a result, RtlAddFunctionTable() wasn't called, so any crashes with v8 jit on the stack on Win x64 caused program termination without giving our crash handler a chance to catch it. R=thakis@chromium.org, jochen@chromium.org BUG=670466,656800,604923,v8:3598 ========== to ========== win64: fix lookup of RtlAddFunctionTable() helper for v8 This helper function moved from chrome.exe to chrome_elf. The failure to look up is unfortunately silent because the same code is used in test binaries. As a result, RtlAddFunctionTable() wasn't called, so any crashes with v8 jit on the stack on Win x64 caused program termination without giving our crash handler a chance to catch it. R=thakis@chromium.org, jochen@chromium.org BUG=670466,656800,604923,v8:3598 ==========
lgtm (I don't understand the connection to v8:3598, but hey) An automated integration-thingy test is coming in a follow-up?
On 2016/12/02 20:01:12, Nico wrote: > lgtm (I don't understand the connection to v8:3598, but hey) v8:3598 was the reason for this helper being added in the first place. It's a bit peripheral I guess. > An automated integration-thingy test is coming in a follow-up? Yeah, all the "crash and check" things use something like about:crash which don't go deep enough into Blink/v8 to catch this case. I need to figure out how to trigger something deeper down that has enough v8 jit on the stack to make Windows get confused and abort normal SEH processing. (Since you're a compiler-y seh person, do you happen to have any insight on https://bugs.chromium.org/p/chromium/issues/detail?id=670466#c22 or #24? It seems like it could still trigger the unhandled exception filter, but decides not to.)
The CQ bit was checked by scottmg@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On Fri, Dec 2, 2016 at 3:24 PM, <scottmg@chromium.org> wrote: > On 2016/12/02 20:01:12, Nico wrote: > > lgtm (I don't understand the connection to v8:3598, but hey) > > v8:3598 was the reason for this helper being added in the first place. > It's a > bit peripheral I guess. > > > An automated integration-thingy test is coming in a follow-up? > > Yeah, all the "crash and check" things use something like about:crash which > don't go deep enough into Blink/v8 to catch this case. I need to figure > out how > to trigger something deeper down that has enough v8 jit on the stack to > make > Windows get confused and abort normal SEH processing. > > (Since you're a compiler-y seh person, do you happen to have any insight on > https://bugs.chromium.org/p/chromium/issues/detail?id=670466#c22 or #24? > It > seems like it could still trigger the unhandled exception filter, but > decides > not to.) > I'd ask rnk next week (he's out today) -- he implemented SEH in clang-cl, he knows it very well :-) (Maybe send him a concrete question, I only read 22 and wasn't quite sure what the question is; then again I didn't try reading the whole bug to reconstruct it.) > > https://codereview.chromium.org/2548653004/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2016/12/02 20:30:47, Nico wrote: > On Fri, Dec 2, 2016 at 3:24 PM, <mailto:scottmg@chromium.org> wrote: > > > On 2016/12/02 20:01:12, Nico wrote: > > > lgtm (I don't understand the connection to v8:3598, but hey) > > > > v8:3598 was the reason for this helper being added in the first place. > > It's a > > bit peripheral I guess. > > > > > An automated integration-thingy test is coming in a follow-up? > > > > Yeah, all the "crash and check" things use something like about:crash which > > don't go deep enough into Blink/v8 to catch this case. I need to figure > > out how > > to trigger something deeper down that has enough v8 jit on the stack to > > make > > Windows get confused and abort normal SEH processing. > > > > (Since you're a compiler-y seh person, do you happen to have any insight on > > https://bugs.chromium.org/p/chromium/issues/detail?id=670466#c22 or #24? > > It > > seems like it could still trigger the unhandled exception filter, but > > decides > > not to.) > > > > I'd ask rnk next week (he's out today) -- he implemented SEH in clang-cl, > he knows it very well :-) (Maybe send him a concrete question, I only read > 22 and wasn't quite sure what the question is; then again I didn't try > reading the whole bug to reconstruct it.) Thanks, will do. > > > > > > https://codereview.chromium.org/2548653004/ > > > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
CQ is committing da patch.
Bot data: {"patchset_id": 1, "attempt_start_ts": 1480710253611450, "parent_rev":
"f7195f771ed53ec9a461ddc06ca0f417d3f24d0b", "commit_rev":
"15e1cef90adbe585a00efc2747c3e54f12858583"}
Message was sent while issue was closed.
Description was changed from ========== win64: fix lookup of RtlAddFunctionTable() helper for v8 This helper function moved from chrome.exe to chrome_elf. The failure to look up is unfortunately silent because the same code is used in test binaries. As a result, RtlAddFunctionTable() wasn't called, so any crashes with v8 jit on the stack on Win x64 caused program termination without giving our crash handler a chance to catch it. R=thakis@chromium.org, jochen@chromium.org BUG=670466,656800,604923,v8:3598 ========== to ========== win64: fix lookup of RtlAddFunctionTable() helper for v8 This helper function moved from chrome.exe to chrome_elf. The failure to look up is unfortunately silent because the same code is used in test binaries. As a result, RtlAddFunctionTable() wasn't called, so any crashes with v8 jit on the stack on Win x64 caused program termination without giving our crash handler a chance to catch it. R=thakis@chromium.org, jochen@chromium.org BUG=670466,656800,604923,v8:3598 ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== win64: fix lookup of RtlAddFunctionTable() helper for v8 This helper function moved from chrome.exe to chrome_elf. The failure to look up is unfortunately silent because the same code is used in test binaries. As a result, RtlAddFunctionTable() wasn't called, so any crashes with v8 jit on the stack on Win x64 caused program termination without giving our crash handler a chance to catch it. R=thakis@chromium.org, jochen@chromium.org BUG=670466,656800,604923,v8:3598 ========== to ========== win64: fix lookup of RtlAddFunctionTable() helper for v8 This helper function moved from chrome.exe to chrome_elf. The failure to look up is unfortunately silent because the same code is used in test binaries. As a result, RtlAddFunctionTable() wasn't called, so any crashes with v8 jit on the stack on Win x64 caused program termination without giving our crash handler a chance to catch it. R=thakis@chromium.org, jochen@chromium.org BUG=670466,656800,604923,v8:3598 Committed: https://crrev.com/56c90a89889693bd6e1a7d7bb4b8f49c69bab2fd Cr-Commit-Position: refs/heads/master@{#436041} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/56c90a89889693bd6e1a7d7bb4b8f49c69bab2fd Cr-Commit-Position: refs/heads/master@{#436041} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
