Bug 111234 - Don't leak Documents when using MutationObserver from extensions
Summary: Don't leak Documents when using MutationObserver from extensions
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Elliott Sprehn
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-03-01 16:47 PST by Elliott Sprehn
Modified: 2013-03-01 18:37 PST (History)
6 users (show)

See Also:


Attachments
Patch (7.12 KB, patch)
2013-03-01 16:52 PST, Elliott Sprehn
no flags Details | Formatted Diff | Diff
Patch (7.41 KB, patch)
2013-03-01 17:13 PST, Elliott Sprehn
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Elliott Sprehn 2013-03-01 16:47:50 PST
Don't leak Documents when using MutationObserver from extensions
Comment 1 Elliott Sprehn 2013-03-01 16:52:30 PST
Created attachment 191072 [details]
Patch
Comment 2 Adam Klein 2013-03-01 17:01:19 PST
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...
Comment 3 Elliott Sprehn 2013-03-01 17:13:17 PST
Created attachment 191079 [details]
Patch
Comment 4 Elliott Sprehn 2013-03-01 17:13:50 PST
(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 5 Adam Barth 2013-03-01 18:00:23 PST
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 6 Elliott Sprehn 2013-03-01 18:14:57 PST
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 7 Adam Barth 2013-03-01 18:22:41 PST
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 8 WebKit Review Bot 2013-03-01 18:37:52 PST
Comment on attachment 191079 [details]
Patch

Clearing flags on attachment: 191079

Committed r144522: <http://trac.webkit.org/changeset/144522>
Comment 9 WebKit Review Bot 2013-03-01 18:37:56 PST
All reviewed patches have been landed.  Closing bug.