116524 2013-05-21 01:39:47 -0700 Active DOM objects stopped twice 2013-05-27 02:12:59 -0700 1 1 1 Unclassified WebKit WebCore Misc. 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 allan.jensen allan.jensen ap commit-queue esprehn+autocc oliver oldest_to_newest 891643 0 allan.jensen 2013-05-21 01:39:47 -0700 ScriptExecutionContext::stopActiveDOMObjects() is called from a few different places. At least two of these are triggered on most application close. The call is probably needed both places, but we could save stopping each dom object, since ScriptExecutionContext remembers all objects should be stopped. This clears up some debugging, but might also help avoid surprises in new active DOM objects. 891645 1 202398 allan.jensen 2013-05-21 01:41:16 -0700 Created attachment 202398 Patch 891853 2 ap 2013-05-21 09:56:39 -0700 Are there any protections against adding new ActiveDOMObjects when m_activeDOMObjectsAreStopped is true? We have lots of code paths that can execute scripts and create new objects in half-closed state, so I'm worried about essentially removing the last pass of stopActiveDOMObjects(). 891903 3 allan.jensen 2013-05-21 11:10:15 -0700 (In reply to comment #2) > Are there any protections against adding new ActiveDOMObjects when m_activeDOMObjectsAreStopped is true? > No, not currently. I added an assert against it before running the tests, I guess it would be a good idea to include that in the patch as well. 892702 4 202649 allan.jensen 2013-05-23 01:45:04 -0700 Created attachment 202649 Patch 892715 5 202653 allan.jensen 2013-05-23 02:45:32 -0700 Created attachment 202653 Patch Fix exposed bug, where in rare cases active dom objects were created after they should have been stopped 892949 6 ap 2013-05-23 10:43:49 -0700 The latest patch still doesn't have the assert, is that intentional? 893016 7 allan.jensen 2013-05-23 13:01:42 -0700 (In reply to comment #6) > The latest patch still doesn't have the assert, is that intentional? Yes. I discovered a case where the assert was triggered. This is in all likelyhood a preexisting bug, but since it was possible to handle the case, I changed the assert to a check that stops active dom elements created after the document has started to stop active dom objects. 893020 8 ap 2013-05-23 13:12:48 -0700 Makes sense. > I discovered a case where the assert was triggered. Can you make a regression test from that? 893250 9 allan.jensen 2013-05-24 03:06:04 -0700 (In reply to comment #8) > Makes sense. > > > I discovered a case where the assert was triggered. > > Can you make a regression test from that? It is already triggered by two regression tests fast/dom/xmlhttprequest-constructor-in-detached-document.html and fast/dom/Window/timer-null-script-execution-context.html. I somehow missed that the first time. 893339 10 202653 ap 2013-05-24 09:51:41 -0700 Comment on attachment 202653 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=202653&action=review r=me if you agree with the comment below, and address it. It's very nice to get rid of unnecessary iterations in release builds, and even better to fix a bug at the same time. > Source/WebCore/ChangeLog:8 > + Only iterate over all active DOM object and stop them once. Please mention how you also fixed a bug with this change. Also how it's covered by existing tests. Have you considered making a test that would demonstrate the fix even in release builds? Just asking here, I'm not sure if that's feasible. > Source/WebCore/dom/ScriptExecutionContext.cpp:216 > + if (m_activeDOMObjectsAreStopped) > + return; This early return basically gets rid of assertions that we used to always run: ASSERT((*iter)->suspendIfNeededCalled()); ASSERT((*iter)->scriptExecutionContext() == this); I think that they are important. Please make sure that they are run in this case, too. 893923 11 allan.jensen 2013-05-27 02:12:59 -0700 Committed r150741: <http://trac.webkit.org/changeset/150741> 202398 2013-05-21 01:41:16 -0700 2013-05-23 01:45:00 -0700 Patch bug-116524-20130521104004.patch text/plain 1479 allan.jensen U3VidmVyc2lvbiBSZXZpc2lvbjogMTUwNDI0CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggMjFiYmE3YWQ5ZTkzZDdh YjgxZGI4NGJmMTQ3ODE4ZWIwZDcyZWMxYS4uNDNhODgwZDkxOTRhZmExZmMxMTU1NjhmYzFkMTRm ZWI3YWFmMDk2OSAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE1IEBACisyMDEzLTA1LTIxICBBbGxh biBTYW5kZmVsZCBKZW5zZW4gIDxhbGxhbi5qZW5zZW5AZGlnaWEuY29tPgorCisgICAgICAgIEFj dGl2ZSBET00gb2JqZWN0cyBzdG9wcGVkIHR3aWNlCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJr aXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xMTY1MjQKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JP RFkgKE9PUFMhKS4KKworICAgICAgICBPbmx5IGl0ZXJhdGUgb3ZlciBhbGwgYWN0aXZlIERPTSBv YmplY3QgYW5kIHN0b3AgdGhlbSBvbmNlLgorCisgICAgICAgICogZG9tL1NjcmlwdEV4ZWN1dGlv bkNvbnRleHQuY3BwOgorICAgICAgICAoV2ViQ29yZTo6U2NyaXB0RXhlY3V0aW9uQ29udGV4dDo6 c3RvcEFjdGl2ZURPTU9iamVjdHMpOgorCiAyMDEzLTA1LTIwICBaYW4gRG9iZXJzZWsgIDx6ZG9i ZXJzZWtAaWdhbGlhLmNvbT4KIAogICAgICAgICBbR1RLXSBGaXggY29uY2F0ZW5hdGlvbnMgb2Yg c3RyaW5nIGxpdGVyYWxzIHRoYXQgYXJlIG5vdCBDKysxMS1jb21wbGlhbnQKZGlmZiAtLWdpdCBh L1NvdXJjZS9XZWJDb3JlL2RvbS9TY3JpcHRFeGVjdXRpb25Db250ZXh0LmNwcCBiL1NvdXJjZS9X ZWJDb3JlL2RvbS9TY3JpcHRFeGVjdXRpb25Db250ZXh0LmNwcAppbmRleCBmMTI3M2NmYWJkZDNh MTQxZjUzMjcyNjc5Njc5NDVlMGQ2YzY4Y2I4Li4zZTMxZThjOGM2Y2JjMDI3YzA0NTQ1ZGZiYTc2 NmYzZmVjYzZlNTA1IDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9kb20vU2NyaXB0RXhlY3V0 aW9uQ29udGV4dC5jcHAKKysrIGIvU291cmNlL1dlYkNvcmUvZG9tL1NjcmlwdEV4ZWN1dGlvbkNv bnRleHQuY3BwCkBAIC0yMDksNiArMjA5LDggQEAgdm9pZCBTY3JpcHRFeGVjdXRpb25Db250ZXh0 OjpyZXN1bWVBY3RpdmVET01PYmplY3RzKCkKIAogdm9pZCBTY3JpcHRFeGVjdXRpb25Db250ZXh0 OjpzdG9wQWN0aXZlRE9NT2JqZWN0cygpCiB7CisgICAgaWYgKG1fYWN0aXZlRE9NT2JqZWN0c0Fy ZVN0b3BwZWQpCisgICAgICAgIHJldHVybjsKICAgICBtX2FjdGl2ZURPTU9iamVjdHNBcmVTdG9w cGVkID0gdHJ1ZTsKICAgICAvLyBObyBwcm90ZWN0aW9uIGFnYWluc3QgbV9hY3RpdmVET01PYmpl Y3RzIGNoYW5naW5nIGR1cmluZyBpdGVyYXRpb246IHN0b3AoKSBzaG91bGRuJ3QgZXhlY3V0ZSBh cmJpdHJhcnkgSlMuCiAgICAgbV9pdGVyYXRpbmdBY3RpdmVET01PYmplY3RzID0gdHJ1ZTsK 202649 2013-05-23 01:45:04 -0700 2013-05-23 02:45:27 -0700 Patch bug-116524-20130523104350.patch text/plain 2020 allan.jensen U3VidmVyc2lvbiBSZXZpc2lvbjogMTUwNTc2CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggMDlmNDUxMDcyYjNiZWI5 N2Q5NTAzYzkzNTcwZDYyMWNiZDYwNWE5ZC4uMWY1NWUyMmIyNmVkZDlkN2I5MmY1Yjk1ZDUwNjUw ZDY5NWVhODMyNyAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE2IEBACisyMDEzLTA1LTIzICBBbGxh biBTYW5kZmVsZCBKZW5zZW4gIDxhbGxhbi5qZW5zZW5AZGlnaWEuY29tPgorCisgICAgICAgIEFj dGl2ZSBET00gb2JqZWN0cyBzdG9wcGVkIHR3aWNlCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJr aXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xMTY1MjQKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JP RFkgKE9PUFMhKS4KKworICAgICAgICBPbmx5IGl0ZXJhdGUgb3ZlciBhbGwgYWN0aXZlIERPTSBv YmplY3QgYW5kIHN0b3AgdGhlbSBvbmNlLgorCisgICAgICAgICogZG9tL1NjcmlwdEV4ZWN1dGlv bkNvbnRleHQuY3BwOgorICAgICAgICAoV2ViQ29yZTo6U2NyaXB0RXhlY3V0aW9uQ29udGV4dDo6 c3RvcEFjdGl2ZURPTU9iamVjdHMpOgorICAgICAgICAoV2ViQ29yZTo6U2NyaXB0RXhlY3V0aW9u Q29udGV4dDo6c3VzcGVuZEFjdGl2ZURPTU9iamVjdElmTmVlZGVkKToKKwogMjAxMy0wNS0yMyAg TWljaGHFgiBQYWt1xYJhIHZlbCBSdXRrYSAgPG0ucGFrdWxhQHNhbXN1bmcuY29tPgogCiAgICAg ICAgIFtFRkxdIEJhY2ssIEZvcndhcmQgYW5kIFN0b3Agc2hvdWxkIGJlIGhpZGRlbiBpbiBjb250 ZXh0IG1lbnUKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL2RvbS9TY3JpcHRFeGVjdXRpb25D b250ZXh0LmNwcCBiL1NvdXJjZS9XZWJDb3JlL2RvbS9TY3JpcHRFeGVjdXRpb25Db250ZXh0LmNw cAppbmRleCBhNWZlZDgwNzhkZWJiNzBjMzc0OGRmYjE3YmVmM2JiOWRjYjk1NjllLi4xNjFjNjFk NTU1YmM1NTc3NjA4MGVmOGUxY2Q1MDNjMmQwZmFhNTgwIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2Vi Q29yZS9kb20vU2NyaXB0RXhlY3V0aW9uQ29udGV4dC5jcHAKKysrIGIvU291cmNlL1dlYkNvcmUv ZG9tL1NjcmlwdEV4ZWN1dGlvbkNvbnRleHQuY3BwCkBAIC0yMTIsNiArMjEyLDggQEAgdm9pZCBT Y3JpcHRFeGVjdXRpb25Db250ZXh0OjpyZXN1bWVBY3RpdmVET01PYmplY3RzKEFjdGl2ZURPTU9i amVjdDo6UmVhc29uRm9yU3UKIAogdm9pZCBTY3JpcHRFeGVjdXRpb25Db250ZXh0OjpzdG9wQWN0 aXZlRE9NT2JqZWN0cygpCiB7CisgICAgaWYgKG1fYWN0aXZlRE9NT2JqZWN0c0FyZVN0b3BwZWQp CisgICAgICAgIHJldHVybjsKICAgICBtX2FjdGl2ZURPTU9iamVjdHNBcmVTdG9wcGVkID0gdHJ1 ZTsKICAgICAvLyBObyBwcm90ZWN0aW9uIGFnYWluc3QgbV9hY3RpdmVET01PYmplY3RzIGNoYW5n aW5nIGR1cmluZyBpdGVyYXRpb246IHN0b3AoKSBzaG91bGRuJ3QgZXhlY3V0ZSBhcmJpdHJhcnkg SlMuCiAgICAgbV9pdGVyYXRpbmdBY3RpdmVET01PYmplY3RzID0gdHJ1ZTsKQEAgLTIzMCw2ICsy MzIsNyBAQCB2b2lkIFNjcmlwdEV4ZWN1dGlvbkNvbnRleHQ6OnN0b3BBY3RpdmVET01PYmplY3Rz KCkKIHZvaWQgU2NyaXB0RXhlY3V0aW9uQ29udGV4dDo6c3VzcGVuZEFjdGl2ZURPTU9iamVjdElm TmVlZGVkKEFjdGl2ZURPTU9iamVjdCogb2JqZWN0KQogewogICAgIEFTU0VSVChtX2FjdGl2ZURP TU9iamVjdHMuY29udGFpbnMob2JqZWN0KSk7CisgICAgQVNTRVJUKCFtX2FjdGl2ZURPTU9iamVj dHNBcmVTdG9wcGVkKTsKICAgICAvLyBFbnN1cmUgYWxsIEFjdGl2ZURPTU9iamVjdHMgYXJlIHN1 c3BlbmRlZCBhbHNvIG5ld2x5IGNyZWF0ZWQgb25lcy4KICAgICBpZiAobV9hY3RpdmVET01PYmpl Y3RzQXJlU3VzcGVuZGVkKQogICAgICAgICBvYmplY3QtPnN1c3BlbmQobV9yZWFzb25Gb3JTdXNw ZW5kaW5nQWN0aXZlRE9NT2JqZWN0cyk7Cg== 202653 2013-05-23 02:45:32 -0700 2013-05-24 09:51:41 -0700 Patch bug-116524-20130523114417.patch text/plain 2015 allan.jensen U3VidmVyc2lvbiBSZXZpc2lvbjogMTUwNTc2CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggMDlmNDUxMDcyYjNiZWI5 N2Q5NTAzYzkzNTcwZDYyMWNiZDYwNWE5ZC4uMWY1NWUyMmIyNmVkZDlkN2I5MmY1Yjk1ZDUwNjUw ZDY5NWVhODMyNyAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE2IEBACisyMDEzLTA1LTIzICBBbGxh biBTYW5kZmVsZCBKZW5zZW4gIDxhbGxhbi5qZW5zZW5AZGlnaWEuY29tPgorCisgICAgICAgIEFj dGl2ZSBET00gb2JqZWN0cyBzdG9wcGVkIHR3aWNlCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJr aXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xMTY1MjQKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JP RFkgKE9PUFMhKS4KKworICAgICAgICBPbmx5IGl0ZXJhdGUgb3ZlciBhbGwgYWN0aXZlIERPTSBv YmplY3QgYW5kIHN0b3AgdGhlbSBvbmNlLgorCisgICAgICAgICogZG9tL1NjcmlwdEV4ZWN1dGlv bkNvbnRleHQuY3BwOgorICAgICAgICAoV2ViQ29yZTo6U2NyaXB0RXhlY3V0aW9uQ29udGV4dDo6 c3RvcEFjdGl2ZURPTU9iamVjdHMpOgorICAgICAgICAoV2ViQ29yZTo6U2NyaXB0RXhlY3V0aW9u Q29udGV4dDo6c3VzcGVuZEFjdGl2ZURPTU9iamVjdElmTmVlZGVkKToKKwogMjAxMy0wNS0yMyAg TWljaGHFgiBQYWt1xYJhIHZlbCBSdXRrYSAgPG0ucGFrdWxhQHNhbXN1bmcuY29tPgogCiAgICAg ICAgIFtFRkxdIEJhY2ssIEZvcndhcmQgYW5kIFN0b3Agc2hvdWxkIGJlIGhpZGRlbiBpbiBjb250 ZXh0IG1lbnUKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL2RvbS9TY3JpcHRFeGVjdXRpb25D b250ZXh0LmNwcCBiL1NvdXJjZS9XZWJDb3JlL2RvbS9TY3JpcHRFeGVjdXRpb25Db250ZXh0LmNw cAppbmRleCBhNWZlZDgwNzhkZWJiNzBjMzc0OGRmYjE3YmVmM2JiOWRjYjk1NjllLi45ZGE1Yjgw YTVhMTFmOGRjNTJiMmU5ZDBlZTE3NTdkMDgzYTBhNjUzIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2Vi Q29yZS9kb20vU2NyaXB0RXhlY3V0aW9uQ29udGV4dC5jcHAKKysrIGIvU291cmNlL1dlYkNvcmUv ZG9tL1NjcmlwdEV4ZWN1dGlvbkNvbnRleHQuY3BwCkBAIC0yMTIsNiArMjEyLDggQEAgdm9pZCBT Y3JpcHRFeGVjdXRpb25Db250ZXh0OjpyZXN1bWVBY3RpdmVET01PYmplY3RzKEFjdGl2ZURPTU9i amVjdDo6UmVhc29uRm9yU3UKIAogdm9pZCBTY3JpcHRFeGVjdXRpb25Db250ZXh0OjpzdG9wQWN0 aXZlRE9NT2JqZWN0cygpCiB7CisgICAgaWYgKG1fYWN0aXZlRE9NT2JqZWN0c0FyZVN0b3BwZWQp CisgICAgICAgIHJldHVybjsKICAgICBtX2FjdGl2ZURPTU9iamVjdHNBcmVTdG9wcGVkID0gdHJ1 ZTsKICAgICAvLyBObyBwcm90ZWN0aW9uIGFnYWluc3QgbV9hY3RpdmVET01PYmplY3RzIGNoYW5n aW5nIGR1cmluZyBpdGVyYXRpb246IHN0b3AoKSBzaG91bGRuJ3QgZXhlY3V0ZSBhcmJpdHJhcnkg SlMuCiAgICAgbV9pdGVyYXRpbmdBY3RpdmVET01PYmplY3RzID0gdHJ1ZTsKQEAgLTIzMyw2ICsy MzUsOCBAQCB2b2lkIFNjcmlwdEV4ZWN1dGlvbkNvbnRleHQ6OnN1c3BlbmRBY3RpdmVET01PYmpl Y3RJZk5lZWRlZChBY3RpdmVET01PYmplY3QqIG9iagogICAgIC8vIEVuc3VyZSBhbGwgQWN0aXZl RE9NT2JqZWN0cyBhcmUgc3VzcGVuZGVkIGFsc28gbmV3bHkgY3JlYXRlZCBvbmVzLgogICAgIGlm IChtX2FjdGl2ZURPTU9iamVjdHNBcmVTdXNwZW5kZWQpCiAgICAgICAgIG9iamVjdC0+c3VzcGVu ZChtX3JlYXNvbkZvclN1c3BlbmRpbmdBY3RpdmVET01PYmplY3RzKTsKKyAgICBpZiAobV9hY3Rp dmVET01PYmplY3RzQXJlU3RvcHBlZCkKKyAgICAgICAgb2JqZWN0LT5zdG9wKCk7CiB9CiAKIHZv aWQgU2NyaXB0RXhlY3V0aW9uQ29udGV4dDo6ZGlkQ3JlYXRlQWN0aXZlRE9NT2JqZWN0KEFjdGl2 ZURPTU9iamVjdCogb2JqZWN0KQo=