56259 2011-03-12 20:20:46 -0800 StorageTracker constructor shouldn't have initialization code and isMainThread() assertion 2011-03-13 12:08:14 -0700 1 1 1 Unclassified WebKit WebKit Misc. 528+ (Nightly build) PC OS X 10.5 RESOLVED FIXED P2 Normal --- 1 adauria adauria alice.barraclough beidson commit-queue oldest_to_newest 366652 0 adauria 2011-03-12 20:20:46 -0800 All initialization code should be in StorageTracker::initializeTracker(). This removes the requirement that StorageTracker constructor must be called on the main thread. 366653 1 85605 adauria 2011-03-12 20:27:11 -0800 Created attachment 85605 Patch 366662 2 alice.barraclough 2011-03-12 22:46:31 -0800 I have just a couple questions. Pardon my lack of understanding of when and where StorageTracker is supposed to exist, but it seems that your patch does not address StorageTracker constructor running on non-main threads, but instead moves the assertion that it's running on the main thread to the initialization of StorageTracker. This seems to me that it doesn't prevent all the non-main threadsfrom creating a StorageTracker instance that doesn't do anything. Maybe i just need a brief overview of StorageTracker ... 366684 3 adauria 2011-03-13 00:21:40 -0800 (In reply to comment #2) > I have just a couple questions. Pardon my lack of understanding of when and where StorageTracker is supposed to exist, but it seems that your patch does not address StorageTracker constructor running on non-main threads, but instead moves the assertion that it's running on the main thread to the initialization of StorageTracker. This seems to me that it doesn't prevent all the non-main threadsfrom creating a StorageTracker instance that doesn't do anything. Maybe i just need a brief overview of StorageTracker ... The constructor started the LocalStorageTask thread. In LocalStorageTask, thread creation isn't protected by a mutex, so it must happen on the main thread. I moved this code out of the constructor to the initialization function, which now must run on the main thread, and so I removed the assert in this patch. 366686 4 adauria 2011-03-13 00:22:56 -0800 (In reply to comment #3) > (In reply to comment #2) > > I have just a couple questions. Pardon my lack of understanding of when and where StorageTracker is supposed to exist, but it seems that your patch does not address StorageTracker constructor running on non-main threads, but instead moves the assertion that it's running on the main thread to the initialization of StorageTracker. This seems to me that it doesn't prevent all the non-main threadsfrom creating a StorageTracker instance that doesn't do anything. Maybe i just need a brief overview of StorageTracker ... > > The constructor started the LocalStorageTask thread. In LocalStorageTask, thread creation isn't protected by a mutex, so it must happen on the main thread. I moved this code out of the constructor to the initialization function, which now must run on the main thread, and so I removed the assert in this patch. So now, it's fine for the constructor to be called on any thread, but the StorageTracker::initializeTracker() must be called on the main thread for the reason stated above. 366687 5 alice.barraclough 2011-03-13 00:25:01 -0800 ok. r=me 366711 6 85605 commit-queue 2011-03-13 12:08:09 -0700 Comment on attachment 85605 Patch Clearing flags on attachment: 85605 Committed r80964: <http://trac.webkit.org/changeset/80964> 366712 7 commit-queue 2011-03-13 12:08:14 -0700 All reviewed patches have been landed. Closing bug. 85605 2011-03-12 20:27:11 -0800 2011-03-13 12:08:09 -0700 Patch bug-56259-20110312202710.patch text/plain 2367 adauria SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDgwOTU2KQorKysgU291cmNlL1dlYkNvcmUvQ2hhbmdl TG9nCSh3b3JraW5nIGNvcHkpCkBAIC0xLDMgKzEsMjAgQEAKKzIwMTEtMDMtMTIgIEFudG9uIEQn QXVyaWEgIDxhZGF1cmlhQGFwcGxlLmNvbT4KKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkg KE9PUFMhKS4KKworICAgICAgICBTdG9yYWdlVHJhY2tlciBjb25zdHJ1Y3RvciBzaG91bGRuJ3Qg aGF2ZSBpbml0aWFsaXphdGlvbiBjb2RlIGFuZCBpc01haW5UaHJlYWQoKSBhc3NlcnRpb24KKyAg ICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTU2MjU5CisKKyAg ICAgICAgTW92ZSBhbGwgU3RvcmFnZVRyYWNrZXIgaW5pdGlhbGl6YXRpb24gdG8KKyAgICAgICAg U3RvcmFnZVRyYWNrZXI6OmluaXRpYWxpemVUcmFja2VyLiBUaGlzIGFsc28gcmVtb3ZlcyB0aGUK KyAgICAgICAgcmVxdWlyZW1lbnQgdGhhdCB0aGUgU3RvcmFnZVRyYWNrZXIgY29uc3RydWN0b3Ig aXNuJ3QgcnVuCisgICAgICAgIG9uIHRoZSBtYWluIHRocmVhZC4KKworICAgICAgICAqIHN0b3Jh Z2UvU3RvcmFnZVRyYWNrZXIuY3BwOgorICAgICAgICAoV2ViQ29yZTo6U3RvcmFnZVRyYWNrZXI6 OmluaXRpYWxpemVUcmFja2VyKToKKyAgICAgICAgKFdlYkNvcmU6OlN0b3JhZ2VUcmFja2VyOjp0 cmFja2VyKToKKyAgICAgICAgKFdlYkNvcmU6OlN0b3JhZ2VUcmFja2VyOjpTdG9yYWdlVHJhY2tl cik6CisKIDIwMTEtMDMtMTIgIERhcmluIEFkbGVyICA8ZGFyaW5AYXBwbGUuY29tPgogCiAgICAg ICAgIFJldmlld2VkIGJ5IERhbiBCZXJuc3RlaW4uCkluZGV4OiBTb3VyY2UvV2ViQ29yZS9zdG9y YWdlL1N0b3JhZ2VUcmFja2VyLmNwcAo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2ViQ29yZS9zdG9y YWdlL1N0b3JhZ2VUcmFja2VyLmNwcAkocmV2aXNpb24gODA5NTYpCisrKyBTb3VyY2UvV2ViQ29y ZS9zdG9yYWdlL1N0b3JhZ2VUcmFja2VyLmNwcAkod29ya2luZyBjb3B5KQpAQCAtNDcsMjEgKzQ3 LDIyIEBAIHN0YXRpYyBTdG9yYWdlVHJhY2tlciogc3RvcmFnZVRyYWNrZXIgPSAKIAogdm9pZCBT dG9yYWdlVHJhY2tlcjo6aW5pdGlhbGl6ZVRyYWNrZXIoY29uc3QgU3RyaW5nJiBzdG9yYWdlUGF0 aCkKIHsKKyAgICBBU1NFUlQoaXNNYWluVGhyZWFkKCkpOwogICAgIEFTU0VSVCghc3RvcmFnZVRy YWNrZXIpOwotICAgIGlmIChzdG9yYWdlVHJhY2tlcikKLSAgICAgICAgcmV0dXJuOwotCi0gICAg c3RvcmFnZVRyYWNrZXIgPSBuZXcgU3RvcmFnZVRyYWNrZXIoc3RvcmFnZVBhdGgpOworICAgIAor ICAgIGlmICghc3RvcmFnZVRyYWNrZXIpCisgICAgICAgIHN0b3JhZ2VUcmFja2VyID0gbmV3IFN0 b3JhZ2VUcmFja2VyKHN0b3JhZ2VQYXRoKTsKKyAgICAKKyAgICBTUUxpdGVGaWxlU3lzdGVtOjpy ZWdpc3RlclNRTGl0ZVZGUygpOwogICAgIHN0b3JhZ2VUcmFja2VyLT5zZXRJc0FjdGl2ZSh0cnVl KTsKKyAgICBzdG9yYWdlVHJhY2tlci0+bV90aHJlYWQtPnN0YXJ0KCk7ICAKICAgICBzdG9yYWdl VHJhY2tlci0+aW1wb3J0T3JpZ2luSWRlbnRpZmllcnMoKTsKIH0KIAogU3RvcmFnZVRyYWNrZXIm IFN0b3JhZ2VUcmFja2VyOjp0cmFja2VyKCkKIHsKLSAgICBpZiAoIXN0b3JhZ2VUcmFja2VyKSB7 CisgICAgaWYgKCFzdG9yYWdlVHJhY2tlcikKICAgICAgICAgc3RvcmFnZVRyYWNrZXIgPSBuZXcg U3RvcmFnZVRyYWNrZXIoIiIpOwotICAgICAgICBzdG9yYWdlVHJhY2tlci0+aW1wb3J0T3JpZ2lu SWRlbnRpZmllcnMoKTsKLSAgICB9CiAgICAgCiAgICAgcmV0dXJuICpzdG9yYWdlVHJhY2tlcjsK IH0KQEAgLTcxLDEzICs3Miw3IEBAIFN0b3JhZ2VUcmFja2VyOjpTdG9yYWdlVHJhY2tlcihjb25z dCBTdHIKICAgICAsIG1fdGhyZWFkKExvY2FsU3RvcmFnZVRocmVhZDo6Y3JlYXRlKCkpCiAgICAg LCBtX2lzQWN0aXZlKGZhbHNlKQogewotICAgIEFTU0VSVChpc01haW5UaHJlYWQoKSk7Ci0gICAg ICAgIAogICAgIHNldFN0b3JhZ2VEaXJlY3RvcnlQYXRoKHN0b3JhZ2VQYXRoKTsKLSAgICAKLSAg ICBTUUxpdGVGaWxlU3lzdGVtOjpyZWdpc3RlclNRTGl0ZVZGUygpOwotICAgIAotICAgIG1fdGhy ZWFkLT5zdGFydCgpOyAgCiB9CiAKIHZvaWQgU3RvcmFnZVRyYWNrZXI6OnNldFN0b3JhZ2VEaXJl Y3RvcnlQYXRoKGNvbnN0IFN0cmluZyYgcGF0aCkK