Bug 135569 - Always clear ConsoleClient when Page/WindowShell is destroyed
Summary: Always clear ConsoleClient when Page/WindowShell is destroyed
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2014-08-04 12:16 PDT by Joseph Pecoraro
Modified: 2014-08-04 14:57 PDT (History)
5 users (show)

See Also:


Attachments
[PATCH] Proposed Fix (1.59 KB, patch)
2014-08-04 12:24 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 2014-08-04 12:16:32 PDT
* 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>
Comment 1 Joseph Pecoraro 2014-08-04 12:24:45 PDT
Created attachment 235979 [details]
[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?
Comment 2 Mark Lam 2014-08-04 13:47:01 PDT
Comment on attachment 235979 [details]
[PATCH] Proposed Fix

r=me
Comment 3 Geoffrey Garen 2014-08-04 14:01:48 PDT
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.)
Comment 4 Joseph Pecoraro 2014-08-04 14:23:12 PDT
(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.
Comment 5 WebKit Commit Bot 2014-08-04 14:57:50 PDT
Comment on attachment 235979 [details]
[PATCH] Proposed Fix

Clearing flags on attachment: 235979

Committed r172006: <http://trac.webkit.org/changeset/172006>
Comment 6 WebKit Commit Bot 2014-08-04 14:57:53 PDT
All reviewed patches have been landed.  Closing bug.