Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
6677034:
Add IndexedDB browser test that causes a crash. (Closed)
|
Created:
9 years, 9 months ago by dgrogan Modified:
9 years, 7 months ago Reviewers:
jorlow CC:
chromium-reviews, jam, darin-cc_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd IndexedDB browser test that causes a crash.
When a frame is unloaded, stop() is called on each ActiveDOMObject.
IDBDatabase::stop can cause IDBDatabase to be destroyed in a roundabout way. Destroying an ActiveDOMObject while they are being iterated over causes a defensive crash.
BUG=75264
TEST=browser_tests --gtest_filter=IndexedDBBrowserTest.DatabaseCallbacksTest
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=78594
Patch Set 1 #Patch Set 2 : remove DISABLED marker on this new test #
Total comments: 13
Patch Set 3 : more explicit js, limit use of expose-gc #
Messages
Total messages: 13 (0 generated)
This is the test for the patch in https://bugs.webkit.org/show_bug.cgi?id=56350
close http://codereview.chromium.org/6677034/diff/2001/content/browser/in_process_w... File content/browser/in_process_webkit/indexed_db_browsertest.cc (right): http://codereview.chromium.org/6677034/diff/2001/content/browser/in_process_w... content/browser/in_process_webkit/indexed_db_browsertest.cc:51: command_line->AppendSwitchASCII(switches::kJavaScriptFlags, "--expose-gc"); Let's only do this on tests we have to. I've seen some bugs not show up when it's on. http://codereview.chromium.org/6677034/diff/2001/content/browser/in_process_w... File content/browser/in_process_webkit/indexed_db_dispatcher_host.cc (right): http://codereview.chromium.org/6677034/diff/2001/content/browser/in_process_w... content/browser/in_process_webkit/indexed_db_dispatcher_host.cc:117: // database_dispatcher_host_ becomes invalid. Add a bug number.
http://codereview.chromium.org/6677034/diff/2001/content/browser/in_process_w... File content/browser/in_process_webkit/indexed_db_browsertest.cc (right): http://codereview.chromium.org/6677034/diff/2001/content/browser/in_process_w... content/browser/in_process_webkit/indexed_db_browsertest.cc:51: command_line->AppendSwitchASCII(switches::kJavaScriptFlags, "--expose-gc"); On 2011/03/15 02:30:04, jorlow wrote: > Let's only do this on tests we have to. I've seen some bugs not show up when > it's on. Sounds good. Is it easy to add this per test in this class or do I need to make a new class that inherits from InProcessBrowserTest?
Also, please make a bug for this and mark it m11. We really need to merge this (and the other bug). http://codereview.chromium.org/6677034/diff/2001/content/browser/in_process_w... File content/browser/in_process_webkit/indexed_db_browsertest.cc (right): http://codereview.chromium.org/6677034/diff/2001/content/browser/in_process_w... content/browser/in_process_webkit/indexed_db_browsertest.cc:51: command_line->AppendSwitchASCII(switches::kJavaScriptFlags, "--expose-gc"); On 2011/03/15 03:32:27, dgrogan wrote: > On 2011/03/15 02:30:04, jorlow wrote: > > Let's only do this on tests we have to. I've seen some bugs not show up when > > it's on. > > Sounds good. Is it easy to add this per test in this class or do I need to make > a new class that inherits from InProcessBrowserTest? I believe you just need another text fixture that inherits from this one.
On Mon, Mar 14, 2011 at 8:35 PM, <jorlow@chromium.org> wrote: > Also, please make a bug for this and mark it m11. We really need to merge > this > (and the other bug). Can I just mark http://code.google.com/p/chromium/issues/detail?id=75264m11? Or do I need to create a new bug? > http://codereview.chromium.org/6677034/ >
New bug...this is a new (but related) issue. On Mon, Mar 14, 2011 at 8:49 PM, David Grogan <dgrogan@chromium.org> wrote: > On Mon, Mar 14, 2011 at 8:35 PM, <jorlow@chromium.org> wrote: > >> Also, please make a bug for this and mark it m11. We really need to merge >> this >> (and the other bug). > > > Can I just mark http://code.google.com/p/chromium/issues/detail?id=75264m11? Or do I need to create a new bug? > > >> http://codereview.chromium.org/6677034/ >> > >
Drive-by with testing comments. http://codereview.chromium.org/6677034/diff/2001/chrome/test/data/indexeddb/d... File chrome/test/data/indexeddb/database_callbacks_first.html (right): http://codereview.chromium.org/6677034/diff/2001/chrome/test/data/indexeddb/d... chrome/test/data/indexeddb/database_callbacks_first.html:3: setTimeout(function() { Is setTimeout really needed here? If yes, could you add comments why? Similarly about gc(). http://codereview.chromium.org/6677034/diff/2001/content/browser/in_process_w... File content/browser/in_process_webkit/indexed_db_browsertest.cc (right): http://codereview.chromium.org/6677034/diff/2001/content/browser/in_process_w... content/browser/in_process_webkit/indexed_db_browsertest.cc:51: command_line->AppendSwitchASCII(switches::kJavaScriptFlags, "--expose-gc"); Can we create a constant for --expose-gc?
http://codereview.chromium.org/6677034/diff/2001/chrome/test/data/indexeddb/d... File chrome/test/data/indexeddb/database_callbacks_first.html (right): http://codereview.chromium.org/6677034/diff/2001/chrome/test/data/indexeddb/d... chrome/test/data/indexeddb/database_callbacks_first.html:3: setTimeout(function() { On 2011/03/15 19:49:11, Paweł Hajdan Jr. wrote: > Is setTimeout really needed here? If yes, could you add comments why? Similarly > about gc(). The gc is...and it makes it more deterministic, so I'm not sure why you're concerned about that. the setTimeout should be given a param of 0 however. And yeah, is it needed? (Pawel, such a timeout is deterministic however, so you shouldn't be concerned from a flake perspective.) http://codereview.chromium.org/6677034/diff/2001/content/browser/in_process_w... File content/browser/in_process_webkit/indexed_db_browsertest.cc (right): http://codereview.chromium.org/6677034/diff/2001/content/browser/in_process_w... content/browser/in_process_webkit/indexed_db_browsertest.cc:51: command_line->AppendSwitchASCII(switches::kJavaScriptFlags, "--expose-gc"); On 2011/03/15 19:49:11, Paweł Hajdan Jr. wrote: > Can we create a constant for --expose-gc? The only reason to bother making a constant is if we replace all instances of the string with it. Does v8 expose its constant for this in any way? If not, then making it a constant really doesn't buy us anything.
http://codereview.chromium.org/6677034/diff/2001/chrome/test/data/indexeddb/d... File chrome/test/data/indexeddb/database_callbacks_first.html (right): http://codereview.chromium.org/6677034/diff/2001/chrome/test/data/indexeddb/d... chrome/test/data/indexeddb/database_callbacks_first.html:3: setTimeout(function() { On 2011/03/15 20:25:46, jorlow wrote: > On 2011/03/15 19:49:11, Paweł Hajdan Jr. wrote: > > Is setTimeout really needed here? If yes, could you add comments why? Yeah, I'll comment these. I think setTimeout makes the IDBRequest returned by webkitIndexedDB.open() available to the garbage collector. The IDBRequest needs to be gone because it has a reference to the IDBDatabase connection object that would prevent it (IDBDatabase object) from being destroyed on stopActiveDOMObjects. I'll do some testing to make sure this is actually the case. > Similarly > > about gc(). > > The gc is...and it makes it more deterministic, so I'm not sure why you're > concerned about that. > > the setTimeout should be given a param of 0 however. And yeah, is it needed? > (Pawel, such a timeout is deterministic however, so you shouldn't be concerned > from a flake perspective.)
http://codereview.chromium.org/6677034/diff/2001/chrome/test/data/indexeddb/d... File chrome/test/data/indexeddb/database_callbacks_first.html (right): http://codereview.chromium.org/6677034/diff/2001/chrome/test/data/indexeddb/d... chrome/test/data/indexeddb/database_callbacks_first.html:3: setTimeout(function() { On 2011/03/15 20:25:46, jorlow wrote: > the setTimeout should be given a param of 0 however. And yeah, is it needed? > (Pawel, such a timeout is deterministic however, so you shouldn't be concerned > from a flake perspective.) I still consider even setTimeout with 0 somewhat problematic. For example, my understanding is that in an onload handler, without setTimeout the function is guaranteed to finish running before the page load finishes. With setTimeout, even with 0 timeout, I think it isn't. Please correct me if I'm wrong. Also, could you explain more why we need to make it more deterministic via gc()? I think I remember some differences between gc being run one or twice (the latter being more thorough), so that's also a bit suspicious for me (again, I'm not familiar with the specifics here).
http://codereview.chromium.org/6677034/diff/2001/chrome/test/data/indexeddb/d... File chrome/test/data/indexeddb/database_callbacks_first.html (right): http://codereview.chromium.org/6677034/diff/2001/chrome/test/data/indexeddb/d... chrome/test/data/indexeddb/database_callbacks_first.html:3: setTimeout(function() { On 2011/03/16 14:53:09, Paweł Hajdan Jr. wrote: > On 2011/03/15 20:25:46, jorlow wrote: > > the setTimeout should be given a param of 0 however. And yeah, is it needed? > > (Pawel, such a timeout is deterministic however, so you shouldn't be concerned > > from a flake perspective.) > > I still consider even setTimeout with 0 somewhat problematic. For example, my > understanding is that in an onload handler, without setTimeout the function is > guaranteed to finish running before the page load finishes. With setTimeout, > even with 0 timeout, I think it isn't. Please correct me if I'm wrong. I don't know if you're right or wrong, but I think it doesn't matter in this case because the function isn't being called in an onload handler, it's being called when the database connection is established. > Also, could you explain more why we need to make it more deterministic via gc()? > I think I remember some differences between gc being run one or twice (the > latter being more thorough), so that's also a bit suspicious for me (again, I'm > not familiar with the specifics here). We're using gc so that the garbage collector will call IDBRequest's destructor. The js in the next patch set is cleaner and commented. The specifics: This test is being used to test a fix for a crash. The crash occurred when stopActiveDOMObjects was called and the only thing holding a RefPtr to IDBDatabase was the chrome indexeddb message dispatcher. Here, idbRequest has a reference to IDBDatabase in its 'result' property, so to reproduce the crash conditions we need to delete the webcore object underlying idbRequest. Otherwise the message dispatcher would not be the lone referrer to IDBDatabase. Without setTimeout, window.event (and potentially other things) have a reference to idbRequest that prevents it from being garbage collected. I say potentially other things because, e.g., setting window.event = 0 doesn't make idbEvent available for garbage collection. Using setTimeout is the only thing I've found that works, but I'm no javascript expert. Suggestions welcome. http://codereview.chromium.org/6677034/diff/2001/content/browser/in_process_w... File content/browser/in_process_webkit/indexed_db_browsertest.cc (right): http://codereview.chromium.org/6677034/diff/2001/content/browser/in_process_w... content/browser/in_process_webkit/indexed_db_browsertest.cc:51: command_line->AppendSwitchASCII(switches::kJavaScriptFlags, "--expose-gc"); On 2011/03/15 02:30:04, jorlow wrote: > Let's only do this on tests we have to. I've seen some bugs not show up when > it's on. Done. http://codereview.chromium.org/6677034/diff/2001/content/browser/in_process_w... File content/browser/in_process_webkit/indexed_db_dispatcher_host.cc (right): http://codereview.chromium.org/6677034/diff/2001/content/browser/in_process_w... content/browser/in_process_webkit/indexed_db_dispatcher_host.cc:117: // database_dispatcher_host_ becomes invalid. On 2011/03/15 02:30:04, jorlow wrote: > Add a bug number. Will do. I'm suddenly having a hard time reproducing (sigh).
David, you can't reproduce because the fix went in upstream. :-) Please just commit this. On Wed, Mar 16, 2011 at 3:00 PM, <dgrogan@chromium.org> wrote: > > > http://codereview.chromium.org/6677034/diff/2001/chrome/test/data/indexeddb/d... > File chrome/test/data/indexeddb/database_callbacks_first.html (right): > > > http://codereview.chromium.org/6677034/diff/2001/chrome/test/data/indexeddb/d... > chrome/test/data/indexeddb/database_callbacks_first.html:3: > setTimeout(function() { > On 2011/03/16 14:53:09, Paweł Hajdan Jr. wrote: > >> On 2011/03/15 20:25:46, jorlow wrote: >> > the setTimeout should be given a param of 0 however. And yeah, is >> > it needed? > >> > (Pawel, such a timeout is deterministic however, so you shouldn't be >> > concerned > >> > from a flake perspective.) >> > > I still consider even setTimeout with 0 somewhat problematic. For >> > example, my > >> understanding is that in an onload handler, without setTimeout the >> > function is > >> guaranteed to finish running before the page load finishes. With >> > setTimeout, > >> even with 0 timeout, I think it isn't. Please correct me if I'm wrong. >> > > I don't know if you're right or wrong, but I think it doesn't matter in > this case because the function isn't being called in an onload handler, > it's being called when the database connection is established. > > > Also, could you explain more why we need to make it more deterministic >> > via gc()? > >> I think I remember some differences between gc being run one or twice >> > (the > >> latter being more thorough), so that's also a bit suspicious for me >> > (again, I'm > >> not familiar with the specifics here). >> > > We're using gc so that the garbage collector will call IDBRequest's > destructor. The js in the next patch set is cleaner and commented. > > The specifics: > This test is being used to test a fix for a crash. The crash occurred > when stopActiveDOMObjects was called and the only thing holding a RefPtr > to IDBDatabase was the chrome indexeddb message dispatcher. > > Here, idbRequest has a reference to IDBDatabase in its 'result' > property, so to reproduce the crash conditions we need to delete the > webcore object underlying idbRequest. Otherwise the message dispatcher > would not be the lone referrer to IDBDatabase. > > Without setTimeout, window.event (and potentially other things) have a > reference to idbRequest that prevents it from being garbage collected. > I say potentially other things because, e.g., setting window.event = 0 > doesn't make idbEvent available for garbage collection. Using > setTimeout is the only thing I've found that works, but I'm no > javascript expert. Suggestions welcome. > > > > http://codereview.chromium.org/6677034/diff/2001/content/browser/in_process_w... > File content/browser/in_process_webkit/indexed_db_browsertest.cc > (right): > > > http://codereview.chromium.org/6677034/diff/2001/content/browser/in_process_w... > content/browser/in_process_webkit/indexed_db_browsertest.cc:51: > command_line->AppendSwitchASCII(switches::kJavaScriptFlags, > "--expose-gc"); > On 2011/03/15 02:30:04, jorlow wrote: > >> Let's only do this on tests we have to. I've seen some bugs not show >> > up when > >> it's on. >> > > Done. > > > http://codereview.chromium.org/6677034/diff/2001/content/browser/in_process_w... > File content/browser/in_process_webkit/indexed_db_dispatcher_host.cc > (right): > > > http://codereview.chromium.org/6677034/diff/2001/content/browser/in_process_w... > content/browser/in_process_webkit/indexed_db_dispatcher_host.cc:117: // > database_dispatcher_host_ becomes invalid. > On 2011/03/15 02:30:04, jorlow wrote: > >> Add a bug number. >> > > Will do. I'm suddenly having a hard time reproducing (sigh). > > > http://codereview.chromium.org/6677034/ >
Thanks for improving the code, removing myself from reviewers. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
