Bug 99674

Summary: Web Inspector: REGRESSION: [JSC] SourceProvider reuses IDs
Product: WebKit Reporter: Timothy Hatcher <timothy>
Component: Web Inspector (Deprecated)Assignee: Oliver Hunt <oliver>
Status: RESOLVED FIXED    
Severity: Normal CC: apavlov, fpizlo, ggaren, keishi, loislo, oliver, pfeldman, pmuellr, roger_fong, vsevik, web-inspector-bugs, webkit-bug-importer, yurys
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch barraclough: review+

Timothy Hatcher
Reported 2012-10-17 19:59:25 PDT
JSC::SourceProvider provides an ID to identify scripts. This ID is simply this cast to an intptr_t. If SourceProvider is freed another one can be allocated in the same location and cause the same ID to be reported to the Debugger. I am able to get the same SourceProvider ID on apple.com by reloading a few times. The IDs a reused during the same page load, so it can really confuse the Inspector given that we use these IDs to identify scripts (especially scripts without URLs.) Either someone needs to retain the SourceProvider (maybe ScriptDebugListener::Script?) so the ID can't be reused or SourceProvider needs a new thread safe unique ID assigned when SourceProvider is created. <rdar://problem/12517297>
Attachments
Patch (2.96 KB, patch)
2013-02-26 16:30 PST, Oliver Hunt 		barraclough: review+
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Timothy Hatcher
Comment 1 2012-10-17 20:14:31 PDT
Retaining SourceProvider will eat more memory since ScriptDebugListener::Script already duplicates the data provided by SourceProvider. Unless we make ScriptDebugListener::Script be a shell for SourceProvider.
Geoffrey Garen
Comment 2 2012-10-17 21:54:38 PDT
We could also have JSC provide a UUID instead of a pointer.
Timothy Hatcher
Comment 3 2012-10-17 22:05:15 PDT
Yeah, that was option two I mentioned in the description: "or SourceProvider needs a new thread safe unique ID assigned when SourceProvider is created."
Timothy Hatcher
Comment 4 2012-10-24 10:11:38 PDT
I am positive this is a regression. I'm seeing the error all the time now when I didn't at all in Safari 6.
Oliver Hunt
Comment 5 2013-01-29 10:57:48 PST
(In reply to comment #4) > I am positive this is a regression. I'm seeing the error all the time now when I didn't at all in Safari 6. What do you mean by thread safe? Same source on different threads needs to provide different ID?
Timothy Hatcher
Comment 6 2013-01-29 11:06:11 PST
(In reply to comment #5) > (In reply to comment #4) > > I am positive this is a regression. I'm seeing the error all the time now when I didn't at all in Safari 6. > > What do you mean by thread safe? Same source on different threads needs to provide different ID? Same source on different threads should get the same ID. I just meant something more than a global counter.
Oliver Hunt
Comment 7 2013-01-29 11:08:25 PST
(In reply to comment #6) > (In reply to comment #5) > > (In reply to comment #4) > > > I am positive this is a regression. I'm seeing the error all the time now when I didn't at all in Safari 6. > > > > What do you mean by thread safe? Same source on different threads needs to provide different ID? > > Same source on different threads should get the same ID. I just meant something more than a global counter. Why would a global counter be insufficient? 2^31 (or 2^63) ids should be more than enough.
Timothy Hatcher
Comment 8 2013-01-29 11:32:47 PST
If there isn't an issue with two threads grabbing the same ID, then nothing.
Oliver Hunt
Comment 9 2013-02-26 16:30:31 PST
Created attachment 190388 [details] Patch
Oliver Hunt
Comment 10 2013-02-26 16:40:00 PST
Committed r144122: <http://trac.webkit.org/changeset/144122>
Note You need to log in before you can comment on or make changes to this bug.