47608 2010-10-13 11:36:35 -0700 Add !SINGLE_THREADED guard. 2010-10-17 17:04:42 -0700 1 1 1 Unclassified WebKit WebCore Misc. 528+ (Nightly build) All All RESOLVED FIXED P2 Normal --- 0 beergun webkit-unassigned ap commit-queue levin oldest_to_newest 293562 0 beergun 2010-10-13 11:36:35 -0700 Single-threaded ports do not have mutex or yield. 293564 1 70639 beergun 2010-10-13 11:38:54 -0700 Created attachment 70639 Patch. Add !SINGLE_THREADED guard. 293972 2 70639 levin 2010-10-13 22:44:50 -0700 Comment on attachment 70639 Patch. Add !SINGLE_THREADED guard. It seems like it would be better to implement a mutex whose tryLock returns true rather than putting these if'defs throughout the code. You could also have a "yield" that does nothing. 294195 3 ap 2010-10-14 10:52:54 -0700 There is already ThreadingNone.cpp - and since it's behind ENABLE(SINGLE_THREADED), I expect it to be present in your port. So, the explanation in ChangeLog is misleading. The reason tryLock() in ThreadingNone.cpp returns false is that we have lots of assertions of this form: ASSERT(!m_databaseGuard.tryLock()). So, even while yield() can just be added to ThreadingNone.cpp, changing tryLock() to support the logic in this function is more involved. Perhaps the approach in this patch is the right one. David, what do you think? 294326 4 levin 2010-10-14 13:40:30 -0700 (In reply to comment #3) > There is already ThreadingNone.cpp - and since it's behind ENABLE(SINGLE_THREADED), I expect it to be present in your port. So, the explanation in ChangeLog is misleading. > > The reason tryLock() in ThreadingNone.cpp returns false is that we have lots of assertions of this form: ASSERT(!m_databaseGuard.tryLock()). > > So, even while yield() can just be added to ThreadingNone.cpp, changing tryLock() to support the logic in this function is more involved. Perhaps the approach in this patch is the right one. David, what do you think? Sounds reasonable. Basically, the simplest thing is to fix the ChangeLog, and that would be sufficent. As a thought exercise, I wonder if the mutex would work PlatformMutex was "bool m_lockTaken". void Mutex::lock() { m_lockTaken = true; } bool Mutex::tryLock() { if (!m_lockTaken) return true; return false; } void Mutex::unlock() { m_lockTaken = false; } Of course this means that there is an extra bool of memory consumed (and I little more code compiled). I don't know how much that is a factor. 294489 5 70807 beergun 2010-10-14 17:30:10 -0700 Created attachment 70807 patch Add dummy yield() for SINGLE_THREADED. 294499 6 levin 2010-10-14 17:43:48 -0700 (In reply to comment #5) > Created an attachment (id=70807) [details] > patch > > Add dummy yield() for SINGLE_THREADED. Alexey pointed out that this won't work for you. You either need to go with the old patch and fix the ChangeLog or fix the mutex class for this case I think the following would work, but you'd need to try it out: In JavaScriptCore/wtf/ThreadingPrimitives.h Change typedef void* PlatformMutex; to typedef bool PlatformMutex; // Use m_mutex to indicate if the mutex is "held" or not. Mutex::Mutex() : m_mutex(false) { } void Mutex::lock() { m_mutex = true; } bool Mutex::tryLock() { if (m_mutex) return false; m_mutex = true; return true; } void Mutex::unlock() { m_mutex = false; } 294500 7 70807 levin 2010-10-14 17:44:13 -0700 Comment on attachment 70807 patch As noted above. 294502 8 70811 beergun 2010-10-14 17:48:59 -0700 Created attachment 70811 patch Changed ChangeLog. 294574 9 70811 ap 2010-10-14 22:00:09 -0700 Comment on attachment 70811 patch + For SINGLE_THREADED ports LockingMutex.tryLock() returns false. + This will make interrupt() fall into infinite loop. A slightly better way to say this would be "This was making interrupt() fall into infinite loop". The current text sounds like your patch introduces falling into infinite loop. But it's OK. 294575 10 70811 commit-queue 2010-10-14 22:01:26 -0700 Comment on attachment 70811 patch Rejecting patch 70811 from commit-queue. Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=abarth-cq-sl', 'apply-attachment', '--force-clean', '--non-interactive', '--quiet', 70811]" exit_code: 2 Cleaning working directory Updating working directory Logging in as commit-queue@webkit.org... Fetching: https://bugs.webkit.org/attachment.cgi?id=70811&action=edit Fetching: https://bugs.webkit.org/show_bug.cgi?id=47608&ctype=xml Processing 1 patch from 1 bug. Processing patch 70811 from bug 47608. Failed to run "[u'/Users/abarth/git/webkit-queue/WebKitTools/Scripts/svn-apply', u'--reviewer', u'Alexey Proskuryakov', u'--force']" exit_code: 2 Full output: http://queues.webkit.org/results/4408033 294586 11 levin 2010-10-14 22:45:56 -0700 The commit queue didn't work because the patch is messed up in the ChangeLog portion. From the style bot: Failed to run "[u'/mnt/git/webkit-chromium-ews/WebKitTools/Scripts/svn-apply', u'--force']" exit_code: 2 Parsed 2 diffs from patch file(s). patching file WebCore/ChangeLog patch: **** malformed patch at line 21: Reviewed by Steve Block. Hyung, you are best off fixing the patch in my opinion. 294790 12 70878 beergun 2010-10-15 09:49:50 -0700 Created attachment 70878 patch Fix ChangeLog conflict, and change ChangeLog to explain better. 295297 13 70878 levin 2010-10-17 16:50:42 -0700 Comment on attachment 70878 patch ok 295299 14 70878 commit-queue 2010-10-17 17:04:36 -0700 Comment on attachment 70878 patch Clearing flags on attachment: 70878 Committed r69935: <http://trac.webkit.org/changeset/69935> 295300 15 commit-queue 2010-10-17 17:04:42 -0700 All reviewed patches have been landed. Closing bug. 70639 2010-10-13 11:38:54 -0700 2010-10-14 17:30:10 -0700 Patch. Add !SINGLE_THREADED guard. patch text/plain 1241 beergun SW5kZXg6IFdlYkNvcmUvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUvQ2hhbmdlTG9n CShyZXZpc2lvbiA2OTY3NikKKysrIFdlYkNvcmUvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBA IC0xLDMgKzEsMTUgQEAKKzIwMTAtMTAtMTMgIEh5dW5nIFNvbmcgIDxiZWVyZ3VuQGNvbXBhbnkx MDAubmV0PgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAg IEFkZCAhU0lOR0xFX1RIUkVBREVEIGd1YXJkLgorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0 Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9NDc2MDgKKworICAgICAgICBTaW5nbGUtdGhyZWFkZWQgcG9y dHMgZG8gbm90IGhhdmUgbXV0ZXggb3IgeWllbGQuCisgICAgICAgIAorICAgICAgICAqIHBsYXRm b3JtL3NxbC9TUUxpdGVEYXRhYmFzZS5jcHA6CisgICAgICAgIChXZWJDb3JlOjpTUUxpdGVEYXRh YmFzZTo6aW50ZXJydXB0KToKKwogMjAxMC0xMC0xMyAgSm9obiBLbm90dGVuYmVsdCAgPGprbm90 dGVuQGNocm9taXVtLm9yZz4KIAogICAgICAgICBSZXZpZXdlZCBieSBTdGV2ZSBCbG9jay4KSW5k ZXg6IFdlYkNvcmUvcGxhdGZvcm0vc3FsL1NRTGl0ZURhdGFiYXNlLmNwcAo9PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0t LSBXZWJDb3JlL3BsYXRmb3JtL3NxbC9TUUxpdGVEYXRhYmFzZS5jcHAJKHJldmlzaW9uIDY5Njc2 KQorKysgV2ViQ29yZS9wbGF0Zm9ybS9zcWwvU1FMaXRlRGF0YWJhc2UuY3BwCSh3b3JraW5nIGNv cHkpCkBAIC0xMDAsNiArMTAwLDcgQEAgdm9pZCBTUUxpdGVEYXRhYmFzZTo6Y2xvc2UoKQogCiB2 b2lkIFNRTGl0ZURhdGFiYXNlOjppbnRlcnJ1cHQoKQogeworI2lmICFFTkFCTEUoU0lOR0xFX1RI UkVBREVEKQogICAgIG1faW50ZXJydXB0ZWQgPSB0cnVlOwogICAgIHdoaWxlICghbV9sb2NraW5n TXV0ZXgudHJ5TG9jaygpKSB7CiAgICAgICAgIE11dGV4TG9ja2VyIGxvY2tlcihtX2RhdGFiYXNl Q2xvc2luZ011dGV4KTsKQEAgLTExMCw2ICsxMTEsNyBAQCB2b2lkIFNRTGl0ZURhdGFiYXNlOjpp bnRlcnJ1cHQoKQogICAgIH0KIAogICAgIG1fbG9ja2luZ011dGV4LnVubG9jaygpOworI2VuZGlm CiB9CiAKIGJvb2wgU1FMaXRlRGF0YWJhc2U6OmlzSW50ZXJydXB0ZWQoKQo= 70807 2010-10-14 17:30:10 -0700 2010-10-14 17:44:12 -0700 patch yield.patch text/plain 1049 beergun SW5kZXg6IEphdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBKYXZhU2NyaXB0 Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDY5ODI0KQorKysgSmF2YVNjcmlwdENvcmUvQ2hhbmdl TG9nCSh3b3JraW5nIGNvcHkpCkBAIC0xLDMgKzEsMTMgQEAKKzIwMTAtMTAtMTQgIEh5dW5nIFNv bmcgIDxiZWVyZ3VuQGNvbXBhbnkxMDAubmV0PgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9E WSAoT09QUyEpLgorCisgICAgICAgIEFkZCBkdW1teSB5aWVsZCgpIGZvciBTSU5HTEVfVEhSRUFE RUQuCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD00NzYw OAorCisgICAgICAgICogd3RmL1RocmVhZGluZ05vbmUuY3BwOgorICAgICAgICAoV1RGOjp5aWVs ZCk6CisKIDIwMTAtMTAtMTQgIE5pa29sYXMgWmltbWVybWFubiAgPG56aW1tZXJtYW5uQHJpbS5j b20+CiAKICAgICAgICAgUmV2aWV3ZWQgYnkgR2F2aW4gQmFycmFjbG91Z2guCkluZGV4OiBKYXZh U2NyaXB0Q29yZS93dGYvVGhyZWFkaW5nTm9uZS5jcHAKPT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gSmF2YVNjcmlw dENvcmUvd3RmL1RocmVhZGluZ05vbmUuY3BwCShyZXZpc2lvbiA2OTgyMykKKysrIEphdmFTY3Jp cHRDb3JlL3d0Zi9UaHJlYWRpbmdOb25lLmNwcAkod29ya2luZyBjb3B5KQpAQCAtNDEsNiArNDEs NyBAQCBpbnQgd2FpdEZvclRocmVhZENvbXBsZXRpb24oVGhyZWFkSWRlbnRpCiB2b2lkIGRldGFj aFRocmVhZChUaHJlYWRJZGVudGlmaWVyKSB7IH0KIFRocmVhZElkZW50aWZpZXIgY3VycmVudFRo cmVhZCgpIHsgcmV0dXJuIFRocmVhZElkZW50aWZpZXIoKTsgfQogYm9vbCBpc01haW5UaHJlYWQo KSB7IHJldHVybiB0cnVlOyB9Cit2b2lkIHlpZWxkKCkgeyB9CiAKIE11dGV4OjpNdXRleCgpIHsg fQogTXV0ZXg6On5NdXRleCgpIHsgfQo= 70811 2010-10-14 17:48:59 -0700 2010-10-15 09:49:50 -0700 patch Add-SINGLE_THREAD-guard.patch text/plain 1316 beergun SW5kZXg6IFdlYkNvcmUvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUvQ2hhbmdlTG9n CShyZXZpc2lvbiA2OTY3NikKKysrIFdlYkNvcmUvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBA IC0xLDMgKzEsMTUgQEAKKzIwMTAtMTAtMTMgIEh5dW5nIFNvbmcgIDxiZWVyZ3VuQGNvbXBhbnkx MDAubmV0PgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAg IEFkZCAhU0lOR0xFX1RIUkVBREVEIGd1YXJkLgorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0 Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9NDc2MDgKKworICAgICAgICBGb3IgU0lOR0xFX1RIUkVBREVE IHBvcnRzIExvY2tpbmdNdXRleC50cnlMb2NrKCkgcmV0dXJucyBmYWxzZS4KKyAgICAgICAgVGhp cyB3aWxsIG1ha2UgaW50ZXJydXB0KCkgZmFsbCBpbnRvIGluZmluaXRlIGxvb3AuCisgICAgICAg IAorICAgICAgICAqIHBsYXRmb3JtL3NxbC9TUUxpdGVEYXRhYmFzZS5jcHA6CisgICAgICAgIChX ZWJDb3JlOjpTUUxpdGVEYXRhYmFzZTo6aW50ZXJydXB0KToKKwogMjAxMC0xMC0xMyAgSm9obiBL bm90dGVuYmVsdCAgPGprbm90dGVuQGNocm9taXVtLm9yZz4KIAogICAgICAgICBSZXZpZXdlZCBi eSBTdGV2ZSBCbG9jay4KSW5kZXg6IFdlYkNvcmUvcGxhdGZvcm0vc3FsL1NRTGl0ZURhdGFiYXNl LmNwcAo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09Ci0tLSBXZWJDb3JlL3BsYXRmb3JtL3NxbC9TUUxpdGVEYXRhYmFzZS5j cHAJKHJldmlzaW9uIDY5Njc2KQorKysgV2ViQ29yZS9wbGF0Zm9ybS9zcWwvU1FMaXRlRGF0YWJh c2UuY3BwCSh3b3JraW5nIGNvcHkpCkBAIC0xMDAsNiArMTAwLDcgQEAgdm9pZCBTUUxpdGVEYXRh YmFzZTo6Y2xvc2UoKQogCiB2b2lkIFNRTGl0ZURhdGFiYXNlOjppbnRlcnJ1cHQoKQogeworI2lm ICFFTkFCTEUoU0lOR0xFX1RIUkVBREVEKQogICAgIG1faW50ZXJydXB0ZWQgPSB0cnVlOwogICAg IHdoaWxlICghbV9sb2NraW5nTXV0ZXgudHJ5TG9jaygpKSB7CiAgICAgICAgIE11dGV4TG9ja2Vy IGxvY2tlcihtX2RhdGFiYXNlQ2xvc2luZ011dGV4KTsKQEAgLTExMCw2ICsxMTEsNyBAQCB2b2lk IFNRTGl0ZURhdGFiYXNlOjppbnRlcnJ1cHQoKQogICAgIH0KIAogICAgIG1fbG9ja2luZ011dGV4 LnVubG9jaygpOworI2VuZGlmCiB9CiAKIGJvb2wgU1FMaXRlRGF0YWJhc2U6OmlzSW50ZXJydXB0 ZWQoKQo= 70878 2010-10-15 09:49:50 -0700 2010-10-17 17:04:36 -0700 patch fix_changelog_conflict.patch text/plain 1322 beergun SW5kZXg6IFdlYkNvcmUvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUvQ2hhbmdlTG9n CShyZXZpc2lvbiA2OTg1NykKKysrIFdlYkNvcmUvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBA IC0xLDMgKzEsMTYgQEAKKzIwMTAtMTAtMTUgIEh5dW5nIFNvbmcgIDxiZWVyZ3VuQGNvbXBhbnkx MDAubmV0PgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAg IEFkZCAhU0lOR0xFX1RIUkVBREVEIGd1YXJkLgorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0 Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9NDc2MDgKKworICAgICAgICBGb3IgU0lOR0xFX1RIUkVBREVE IHBvcnRzIExvY2tpbmdNdXRleC50cnlMb2NrKCkgcmV0dXJucyBmYWxzZS4KKyAgICAgICAgVGhp cyB3aWxsIHByZXZlbnQgaW50ZXJydXB0KCkgZnJvbSBmYWxsaW5nIGludG8gaW5maW5pdGUgbG9v cC4KKworICAgICAgICAqIHBsYXRmb3JtL3NxbC9TUUxpdGVEYXRhYmFzZS5jcHA6CisgICAgICAg IChXZWJDb3JlOjpTUUxpdGVEYXRhYmFzZTo6aW50ZXJydXB0KToKKwogMjAxMC0xMC0xNSAgSWx5 YSBUaWtob25vdnNreSAgPGxvaXNsb0BjaHJvbWl1bS5vcmc+CiAKICAgICAgICAgUmV2aWV3ZWQg YnkgWXVyeSBTZW1pa2hhdHNreS4KSW5kZXg6IFdlYkNvcmUvcGxhdGZvcm0vc3FsL1NRTGl0ZURh dGFiYXNlLmNwcAo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09Ci0tLSBXZWJDb3JlL3BsYXRmb3JtL3NxbC9TUUxpdGVEYXRh YmFzZS5jcHAJKHJldmlzaW9uIDY5ODU3KQorKysgV2ViQ29yZS9wbGF0Zm9ybS9zcWwvU1FMaXRl RGF0YWJhc2UuY3BwCSh3b3JraW5nIGNvcHkpCkBAIC0xMDEsNiArMTAxLDcgQEAgdm9pZCBTUUxp dGVEYXRhYmFzZTo6Y2xvc2UoKQogCiB2b2lkIFNRTGl0ZURhdGFiYXNlOjppbnRlcnJ1cHQoKQog eworI2lmICFFTkFCTEUoU0lOR0xFX1RIUkVBREVEKQogICAgIG1faW50ZXJydXB0ZWQgPSB0cnVl OwogICAgIHdoaWxlICghbV9sb2NraW5nTXV0ZXgudHJ5TG9jaygpKSB7CiAgICAgICAgIE11dGV4 TG9ja2VyIGxvY2tlcihtX2RhdGFiYXNlQ2xvc2luZ011dGV4KTsKQEAgLTExMSw2ICsxMTIsNyBA QCB2b2lkIFNRTGl0ZURhdGFiYXNlOjppbnRlcnJ1cHQoKQogICAgIH0KIAogICAgIG1fbG9ja2lu Z011dGV4LnVubG9jaygpOworI2VuZGlmCiB9CiAKIGJvb2wgU1FMaXRlRGF0YWJhc2U6OmlzSW50 ZXJydXB0ZWQoKQo=