135569 2014-08-04 12:16:32 -0700 Always clear ConsoleClient when Page/WindowShell is destroyed 2014-08-04 14:57:53 -0700 1 1 1 Unclassified WebKit WebCore Misc. 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 joepeck webkit-unassigned commit-queue ggaren joepeck mark.lam timothy oldest_to_newest 1026662 0 joepeck 2014-08-04 12:16:32 -0700 * SUMMARY WebCore::Page's set the ConsoleClient to their PageConsole object. It should always clear this pointer whenever the PageConsole is going away. Otherwise we could crash trying to use it. Thread 0 Crashed:: main Dispatch queue: com.apple.main-thread 0 ??? 000000000000000000 0 + 0 1 com.apple.JavaScriptCore 0x7fff973a9ca2 JSC::ConsoleClient::logWithLevel 2 com.apple.JavaScriptCore 0x7fff973a8f3e JSC::consoleLogWithLevel 3 ??? 0x000042ce8dc01114 0 + 73454908870932 4 com.apple.JavaScriptCore 0x7fff973a5fbe llint_entry 5 com.apple.JavaScriptCore 0x7fff973a04c1 callToJavaScript <rdar://problem/17856494> 1026663 1 235979 joepeck 2014-08-04 12:24:45 -0700 Created attachment 235979 [PATCH] Proposed Fix I spent a bit of time trying to create a test for this. I have a manual test case, but it has two setTimeouts of 100ms, which I can't seem to reduce by much. Is that worth adding? 1026680 2 235979 mark.lam 2014-08-04 13:47:01 -0700 Comment on attachment 235979 [PATCH] Proposed Fix r=me 1026686 3 ggaren 2014-08-04 14:01:48 -0700 Usually, it is the client's responsibility to clear this pointer, usually in the client object's destructor. Would that approach work here? (I think this patch is fine too, but it would be a bit cleaner for the client to clear its own pointer.) 1026687 4 joepeck 2014-08-04 14:23:12 -0700 (In reply to comment #3) > Usually, it is the client's responsibility to clear this pointer, usually in the client object's destructor. Would that approach work here? > > (I think this patch is fine too, but it would be a bit cleaner for the client to clear its own pointer.) This is a little more complicated than that. During the lifetime of a page, it seems there may be the potential for multiple DOMWindow objects / JSGlobalObjects. As these come and go (page navigation) we want to make sure we configure the ConsoleClient in each of these to be the PageConsole. Currently this happens in WindowShell initialization and clearing in ScriptController. With one other place in ScriptCachedFrameData::restore when a page is created from the page cache. Here we are fixing an overlooked case in destruction (which doesn't go through the normal clearing path). I agree, this is messy and should be made clearer somehow. 1026694 5 235979 commit-queue 2014-08-04 14:57:50 -0700 Comment on attachment 235979 [PATCH] Proposed Fix Clearing flags on attachment: 235979 Committed r172006: <http://trac.webkit.org/changeset/172006> 1026695 6 commit-queue 2014-08-04 14:57:53 -0700 All reviewed patches have been landed. Closing bug. 235979 2014-08-04 12:24:45 -0700 2014-08-04 14:57:50 -0700 [PATCH] Proposed Fix console-client.patch text/plain 1626 joepeck ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZyBiL1NvdXJjZS9XZWJDb3JlL0No YW5nZUxvZwppbmRleCBjZWNiMmJkLi4xYWE5NGU0IDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29y ZS9DaGFuZ2VMb2cKKysrIGIvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTUg QEAKKzIwMTQtMDgtMDQgIEpvc2VwaCBQZWNvcmFybyAgPHBlY29yYXJvQGFwcGxlLmNvbT4KKwor ICAgICAgICBBbHdheXMgY2xlYXIgQ29uc29sZUNsaWVudCB3aGVuIFBhZ2UvV2luZG93U2hlbGwg aXMgZGVzdHJveWVkCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNn aT9pZD0xMzU1NjkKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAg ICAgICAqIGJpbmRpbmdzL2pzL1NjcmlwdENvbnRyb2xsZXIuY3BwOgorICAgICAgICAoV2ViQ29y ZTo6U2NyaXB0Q29udHJvbGxlcjo6flNjcmlwdENvbnRyb2xsZXIpOgorICAgICAgICBXaGVuZXZl ciBhIHdpbmRvdyBzaGVsbCBnb2VzIGF3YXksIGNsZWFyIHRoZSBjb25zb2xlIGNsaWVudC4KKyAg ICAgICAgV2UgZGlkIHRoaXMgaW4gY2xlYXJXaW5kb3dTaGVsbCBidXQgbm90IGJlZm9yZSBkZXN0 cm95aW5nLgorCiAyMDE0LTA4LTA0ICBDaHJpcyBGbGVpemFjaCAgPGNmbGVpemFjaEBhcHBsZS5j b20+CiAKICAgICAgICAgQVg6IFNlbGVjdFRleHQgZnVuY3Rpb25hbGl0eSBhbHdheXMgc2VsZWN0 cyB0ZXh0IGFmdGVyIGN1cnJlbnQgc2VsZWN0aW9uIGV2ZW4gaWYgY2xvc2VyIHNlbGVjdGlvbiBp cyBiZWhpbmQgaXQKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL2JpbmRpbmdzL2pzL1Njcmlw dENvbnRyb2xsZXIuY3BwIGIvU291cmNlL1dlYkNvcmUvYmluZGluZ3MvanMvU2NyaXB0Q29udHJv bGxlci5jcHAKaW5kZXggNDQxYjcyNy4uNGJiNTZmNyAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNv cmUvYmluZGluZ3MvanMvU2NyaXB0Q29udHJvbGxlci5jcHAKKysrIGIvU291cmNlL1dlYkNvcmUv YmluZGluZ3MvanMvU2NyaXB0Q29udHJvbGxlci5jcHAKQEAgLTk0LDggKzk0LDExIEBAIFNjcmlw dENvbnRyb2xsZXI6On5TY3JpcHRDb250cm9sbGVyKCkKIAogICAgIC8vIEl0J3MgbGlrZWx5IHRo YXQgZGVzdHJveWluZyBtX3dpbmRvd1NoZWxscyB3aWxsIGNyZWF0ZSBhIGxvdCBvZiBnYXJiYWdl LgogICAgIGlmICghbV93aW5kb3dTaGVsbHMuaXNFbXB0eSgpKSB7Ci0gICAgICAgIHdoaWxlICgh bV93aW5kb3dTaGVsbHMuaXNFbXB0eSgpKQotICAgICAgICAgICAgZGVzdHJveVdpbmRvd1NoZWxs KCptX3dpbmRvd1NoZWxscy5iZWdpbigpLT5rZXkpOworICAgICAgICB3aGlsZSAoIW1fd2luZG93 U2hlbGxzLmlzRW1wdHkoKSkgeworICAgICAgICAgICAgU2hlbGxNYXA6Oml0ZXJhdG9yIGl0ZXIg PSBtX3dpbmRvd1NoZWxscy5iZWdpbigpOworICAgICAgICAgICAgaXRlci0+dmFsdWUtPndpbmRv dygpLT5zZXRDb25zb2xlQ2xpZW50KG51bGxwdHIpOworICAgICAgICAgICAgZGVzdHJveVdpbmRv d1NoZWxsKCppdGVyLT5rZXkpOworICAgICAgICB9CiAgICAgICAgIGdjQ29udHJvbGxlcigpLmdh cmJhZ2VDb2xsZWN0U29vbigpOwogICAgIH0KIH0K