Bug 172768

Created attachment 311620 [details] Patch

Comment on attachment 311620 [details] Patch Attachment 311620 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/3849987 New failing tests: fast/css/target-fragment-match.html

Created attachment 311633 [details] Archive of layout-test-results from ews123 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews123 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.5

Comment on attachment 311620 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=311620&action=review > Source/WTF/wtf/StackTrace.cpp:59 > + if (!numberOfFrames) > + return nullptr; Not sure why this is OK. The StackTrace object in the local variable "trace" will leak every time this is called.

Comment on attachment 311620 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=311620&action=review >> Source/WTF/wtf/StackTrace.cpp:59 >> + return nullptr; > > Not sure why this is OK. The StackTrace object in the local variable "trace" will leak every time this is called. You are right here. It's not Ok due the leak.

Created attachment 313461 [details] Patch

Comment on attachment 313461 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=313461&action=review This change doesn’t make things worse, but this existing code does things wrong. It’s not correct to allocate an object with placement new in memory that was allocated with fastMalloc and then later delete that object with a call to delete. Given how it’s allocated, the correct way to destroy that object is to explicitly call the destructor and then use fastFree to release the memory. I’d like to see captureStackTrace use std::unique_ptr or some other smart pointer for the return value so callers aren’t likely to get that wrong. > Source/WTF/wtf/StackTrace.cpp:62 > + delete trace; As described above, this is incorrect, although the existing calling code outside the function has the same problem. Correct code for this would be: trace->~StackTrace(); fastFree(trace);

Comment on attachment 313461 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=313461&action=review >> Source/WTF/wtf/StackTrace.cpp:62 >> + delete trace; > > As described above, this is incorrect, although the existing calling code outside the function has the same problem. Correct code for this would be: > > trace->~StackTrace(); > fastFree(trace); What about making StackTrace WTF_MAKE_FAST_ALLOCATED and leaving in this as delete, and changing how we ‘new’ above?

(In reply to Darin Adler from comment #7) > Comment on attachment 313461 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=313461&action=review > > This change doesn’t make things worse, but this existing code does things > wrong. It’s not correct to allocate an object with placement new in memory > that was allocated with fastMalloc and then later delete that object with a > call to delete. Given how it’s allocated, the correct way to destroy that > object is to explicitly call the destructor and then use fastFree to release > the memory. I’d like to see captureStackTrace use std::unique_ptr or some > other smart pointer for the return value so callers aren’t likely to get > that wrong. +1 > > > Source/WTF/wtf/StackTrace.cpp:62 > > + delete trace; > > As described above, this is incorrect, although the existing calling code > outside the function has the same problem. Correct code for this would be: > > trace->~StackTrace(); > fastFree(trace);

Comment on attachment 313461 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=313461&action=review >>>> Source/WTF/wtf/StackTrace.cpp:62 >>>> + delete trace; >>> >>> As described above, this is incorrect, although the existing calling code outside the function has the same problem. Correct code for this would be: >>> >>> trace->~StackTrace(); >>> fastFree(trace); >> >> What about making StackTrace WTF_MAKE_FAST_ALLOCATED and leaving in this as delete, and changing how we ‘new’ above? > > I’m not sure that would be straightforward. I believe this uses fastMalloc because it wants to allocate storage past the end of the StackTrace, not for performance reasons.

(In reply to Darin Adler from comment #7) > Comment on attachment 313461 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=313461&action=review > > This change doesn’t make things worse, but this existing code does things > wrong. It’s not correct to allocate an object with placement new in memory > that was allocated with fastMalloc and then later delete that object with a > call to delete. Given how it’s allocated, the correct way to destroy that > object is to explicitly call the destructor and then use fastFree to release > the memory. I’d like to see captureStackTrace use std::unique_ptr or some > other smart pointer for the return value so callers aren’t likely to get > that wrong. > > > Source/WTF/wtf/StackTrace.cpp:62 > > + delete trace; > > As described above, this is incorrect, although the existing calling code > outside the function has the same problem. Correct code for this would be: > > trace->~StackTrace(); > fastFree(trace); In that case should we open a new bug to fix that? In that case, I should place a FIXME and fix that in the right way.

(In reply to Caio Lima from comment #11) > (In reply to Darin Adler from comment #7) > > Comment on attachment 313461 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=313461&action=review > > > > This change doesn’t make things worse, but this existing code does things > > wrong. It’s not correct to allocate an object with placement new in memory > > that was allocated with fastMalloc and then later delete that object with a > > call to delete. Given how it’s allocated, the correct way to destroy that > > object is to explicitly call the destructor and then use fastFree to release > > the memory. I’d like to see captureStackTrace use std::unique_ptr or some > > other smart pointer for the return value so callers aren’t likely to get > > that wrong. > > > > > Source/WTF/wtf/StackTrace.cpp:62 > > > + delete trace; > > > > As described above, this is incorrect, although the existing calling code > > outside the function has the same problem. Correct code for this would be: > > > > trace->~StackTrace(); > > fastFree(trace); > > In that case should we open a new bug to fix that? In that case, I should > place a FIXME and fix that in the right way. I would just make the code do the right thing since the change seems small.

Comment on attachment 313461 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=313461&action=review >>>>>> Source/WTF/wtf/StackTrace.cpp:62 >>>>>> + delete trace; >>>>> >>>>> As described above, this is incorrect, although the existing calling code outside the function has the same problem. Correct code for this would be: >>>>> >>>>> trace->~StackTrace(); >>>>> fastFree(trace); >>>> >>>> What about making StackTrace WTF_MAKE_FAST_ALLOCATED and leaving in this as delete, and changing how we ‘new’ above? >>> >>> >> >> I’m not sure that would be straightforward. I believe this uses fastMalloc because it wants to allocate storage past the end of the StackTrace, not for performance reasons. > > In that case should we open a new bug to fix that? In that case, I should place a FIXME and fix that in the right way. I think we should annotate it with WTF_MAKE_FAST_ALLOCATED. The situation is the same to StringImpl, which may has trailing string content too. By annotating WTF_MAKE_FAST_ALLOCATED, `delete trace` becomes completely valid. And I think we should add `std::unique_ptr<StackTrace> create(int maxFrames)` private static function and use it in captureStackTrace. By doing so, we can avoid explicit `delete`. And I also think that `captureStackTrace` should return `std::unique_ptr<StackTrace>`.

Created attachment 314215 [details] Patch

Comment on attachment 314215 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=314215&action=review r=me with some changes. > Source/WTF/ChangeLog:8 > + In some architectures, like ARMv6 running into Raspberry pi, the typo: into -> on a > Source/WTF/ChangeLog:11 > + runtime crash. typo: a runtime crash > Source/WTF/ChangeLog:13 > + avoid runtime crash in such case. typo: ditto "a runtime crash" > Source/JavaScriptCore/runtime/ExceptionScope.cpp:60 > currentStack->dump(out, " "); I think this should be: if (currentStack) currentStack->dump(out, " "); given that the captureStackTrace function can return a nullptr. I think you also need a null check on the m_vm.nativeStackTraceOfLastThrow()->dump(out, " "); below.

Comment on attachment 314215 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=314215&action=review ExceptionScope::unexpectedExceptionMessage() expects the StackTrace object to always succeed, which I think is a reasonable expectation. There's nothing that says that the StackTrace object has to contain any frames. How about just doing a more minimal change instead (see comment in StackTrace::captureStackTrace() below) ... > Source/WTF/wtf/StackTrace.cpp:72 > + if (!numberOfFrames) > + return nullptr; > + > RELEASE_ASSERT(numberOfFrames >= framesToSkip); > trace->m_size = numberOfFrames - framesToSkip; I suggest undoing everything else, and changing this to: if (numberOfFrames) { RELEASE_ASSERT(numberOfFrames >= framesToSkip); trace->m_size = numberOfFrames - framesToSkip; } else trace->m_size = 0;

Created attachment 314261 [details] Patch

(In reply to Mark Lam from comment #16) > Comment on attachment 314215 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=314215&action=review > > ExceptionScope::unexpectedExceptionMessage() expects the StackTrace object > to always succeed, which I think is a reasonable expectation. There's > nothing that says that the StackTrace object has to contain any frames. How > about just doing a more minimal change instead (see comment in > StackTrace::captureStackTrace() below) ... > > > Source/WTF/wtf/StackTrace.cpp:72 > > + if (!numberOfFrames) > > + return nullptr; > > + > > RELEASE_ASSERT(numberOfFrames >= framesToSkip); > > trace->m_size = numberOfFrames - framesToSkip; > > I suggest undoing everything else, and changing this to: > > if (numberOfFrames) { > RELEASE_ASSERT(numberOfFrames >= framesToSkip); > trace->m_size = numberOfFrames - framesToSkip; > } else > trace->m_size = 0; Done.

Comment on attachment 314261 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=314261&action=review > Source/WTF/wtf/StackTrace.cpp:64 > + trace->m_capacity = maxFrames; This is still incorrect. capacity and size are 2 different concepts. trace->m_capacity needs to be set even if trace->m_size is 0. If you look in StackTrace.hpp, you'll see that trace->m_capacity being set has a special meaning: it means that we did allocate memory for the StackTrace trace (which we did because we ensure that maxFrames = std::max(1, maxFrames) above). In practice, though not setting trace->m_capacity here means that stack() will return a random address, size() being 0 should stop the client from using the buffer. Regardless, let's do this properly and set trace->m_capacity so that we tell the truth about how we allocate the buffer.

Created attachment 314334 [details] Patch

(In reply to Mark Lam from comment #19) > Comment on attachment 314261 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=314261&action=review > > > Source/WTF/wtf/StackTrace.cpp:64 > > + trace->m_capacity = maxFrames; > > This is still incorrect. capacity and size are 2 different concepts. > trace->m_capacity needs to be set even if trace->m_size is 0. If you look > in StackTrace.hpp, you'll see that trace->m_capacity being set has a special > meaning: it means that we did allocate memory for the StackTrace trace > (which we did because we ensure that maxFrames = std::max(1, maxFrames) > above). In practice, though not setting trace->m_capacity here means that > stack() will return a random address, size() being 0 should stop the client > from using the buffer. Regardless, let's do this properly and set > trace->m_capacity so that we tell the truth about how we allocate the buffer. Makes sense. I misread your suggestion. BTW, don't you think that returning raw pointer here is a potential source of memory leak?

Comment on attachment 314334 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=314334&action=review r=me with ChangeLog fix. > Source/WTF/ChangeLog:12 > + This Patch is adding a guard for the case described above to typo: /Patch/patch/.

(In reply to Caio Lima from comment #21) > BTW, don't you think that returning raw pointer here is a potential source > of memory leak? Yes, it is true that there's theoretical risk. However, I recall opting to go this route because there was a reason that makes returning a direct pointer the better solution here. I may have remembered wrongly, or perhaps, the old reason is no longer valid. We may want to revisit this at a later point in time (it's not critical for the present, and not for this patch). If you wish to investigate it carefully and make sure that there are no longer any need that requires returning a direct pointer, then feel free to create a new bug and propose a solution.

Created attachment 314385 [details] Patch for landing Fixed typos

Created attachment 314386 [details] Patch for landing Rebased with master

(In reply to Mark Lam from comment #23) > (In reply to Caio Lima from comment #21) > > BTW, don't you think that returning raw pointer here is a potential source > > of memory leak? > > Yes, it is true that there's theoretical risk. However, I recall opting to > go this route because there was a reason that makes returning a direct > pointer the better solution here. I may have remembered wrongly, or > perhaps, the old reason is no longer valid. We may want to revisit this at a > later point in time (it's not critical for the present, and not for this > patch). If you wish to investigate it carefully and make sure that there > are no longer any need that requires returning a direct pointer, then feel > free to create a new bug and propose a solution. Ok.

Comment on attachment 314386 [details] Patch for landing Clearing flags on attachment: 314386 Committed r219053: <http://trac.webkit.org/changeset/219053>

All reviewed patches have been landed. Closing bug.

On a MIPS system where libunwind has not been compiled, backtrace() seems to always return 1, and I still hit the ASSERT(). Should the test be on numberOfFrames>1 ? Or should the ASSERT() just be an if and not an ASSERT()? I'm not sure I understand the spirit behind that ASSERT.

(In reply to Guillaume Emont from comment #29) > On a MIPS system where libunwind has not been compiled, backtrace() seems to > always return 1, and I still hit the ASSERT(). Should the test be on > numberOfFrames>1 ? Or should the ASSERT() just be an if and not an ASSERT()? > I'm not sure I understand the spirit behind that ASSERT. Your bug here is not with the StackTrace implementation. It is that Platform.h is lying about MIPS having a backtrace implementation. To fix this: change Platform.h to not define HAVE_BACKTRACE for CPU_MIPS.

(In reply to Mark Lam from comment #30) > (In reply to Guillaume Emont from comment #29) > > On a MIPS system where libunwind has not been compiled, backtrace() seems to > > always return 1, and I still hit the ASSERT(). Should the test be on > > numberOfFrames>1 ? Or should the ASSERT() just be an if and not an ASSERT()? > > I'm not sure I understand the spirit behind that ASSERT. > > Your bug here is not with the StackTrace implementation. It is that > Platform.h is lying about MIPS having a backtrace implementation. To fix > this: change Platform.h to not define HAVE_BACKTRACE for CPU_MIPS. I have not tried yet, as I think I need to recompile a whole system for that, but my understanding is that I get a 1 return value because I do not have libunwind, but that if I had libunwind available when building gcc/libgcc, then I would get a properly working backtrace(). In other words: I do think there is support for backtrace() on MIPS, just not on my specific system that lacks libunwind. (All of that pending verification).

(In reply to Guillaume Emont from comment #31) > (In reply to Mark Lam from comment #30) > > (In reply to Guillaume Emont from comment #29) > > > On a MIPS system where libunwind has not been compiled, backtrace() seems to > > > always return 1, and I still hit the ASSERT(). Should the test be on > > > numberOfFrames>1 ? Or should the ASSERT() just be an if and not an ASSERT()? > > > I'm not sure I understand the spirit behind that ASSERT. > > > > Your bug here is not with the StackTrace implementation. It is that > > Platform.h is lying about MIPS having a backtrace implementation. To fix > > this: change Platform.h to not define HAVE_BACKTRACE for CPU_MIPS. > > I have not tried yet, as I think I need to recompile a whole system for > that, but my understanding is that I get a 1 return value because I do not > have libunwind, but that if I had libunwind available when building > gcc/libgcc, then I would get a properly working backtrace(). In other words: > I do think there is support for backtrace() on MIPS, just not on my specific > system that lacks libunwind. (All of that pending verification). Can you file a separate bug to track this for MIPS if needed? Better to hold the MIPS specific discussion there.