| Summary: | Don't leak Documents when using MutationObserver from extensions | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Elliott Sprehn <esprehn> | ||||||
| Component: | New Bugs | Assignee: | Elliott Sprehn <esprehn> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | abarth, adamk, haraken, japhet, rafaelw, webkit.review.bot | ||||||
| Priority: | P2 | ||||||||
| Version: | 528+ (Nightly build) | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| Attachments: |
|
||||||||
|
Description
Elliott Sprehn
2013-03-01 16:47:50 PST
Created attachment 191072 [details]
Patch
Comment on attachment 191072 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=191072&action=review > Source/WebCore/bindings/v8/V8Binding.cpp:273 > + if (!world) Seems like there ought to at least be a FIXME here, since it's not at all obvious why this is a signal for the main world... Created attachment 191079 [details]
Patch
(In reply to comment #2) > (From update of attachment 191072 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=191072&action=review > > > Source/WebCore/bindings/v8/V8Binding.cpp:273 > > + if (!world) > > Seems like there ought to at least be a FIXME here, since it's not at all obvious why this is a signal for the main world... Done. Comment on attachment 191079 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=191079&action=review We should eventually replace all uses of WorldContextHandle with this mechanism. > Source/WebCore/bindings/v8/V8Binding.cpp:278 > + return v8::Local<v8::Context>::New(frame->script()->windowShell(world)->context()); It it possible that windowShell(world) will return null? I guess not if the context is active in the frame. > Source/WebCore/bindings/v8/V8Binding.cpp:282 > + if (WorkerScriptController* script = static_cast<WorkerContext*>(context)->script()) Is it possible for script() to return null here? That seems like it would be an odd situation... Comment on attachment 191079 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=191079&action=review >> Source/WebCore/bindings/v8/V8Binding.cpp:278 >> + return v8::Local<v8::Context>::New(frame->script()->windowShell(world)->context()); > > It it possible that windowShell(world) will return null? I guess not if the context is active in the frame. It's not possible if you look at the code in ScriptController::windowShell which crashes if the shell would have been null since it does !shell->isContextInitialized() && shell->initializeIfNeeded() in an if statement below the find(). There should probably be an ASSERT in there to make this more obvious. >> Source/WebCore/bindings/v8/V8Binding.cpp:282 >> + if (WorkerScriptController* script = static_cast<WorkerContext*>(context)->script()) > > Is it possible for script() to return null here? That seems like it would be an odd situation... The other toV8Context and WorldContextHandle assume this is impossible. Comment on attachment 191079 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=191079&action=review >>> Source/WebCore/bindings/v8/V8Binding.cpp:282 >>> + if (WorkerScriptController* script = static_cast<WorkerContext*>(context)->script()) >> >> Is it possible for script() to return null here? That seems like it would be an odd situation... > > The other toV8Context and WorldContextHandle assume this is impossible. You probably mean "is possible". :) Comment on attachment 191079 [details] Patch Clearing flags on attachment: 191079 Committed r144522: <http://trac.webkit.org/changeset/144522> All reviewed patches have been landed. Closing bug. |