Bug 111234 – Don't leak Documents when using MutationObserver from extensions

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Elliott Sprehn]

Don't leak Documents when using MutationObserver from extensions

[Elliott Sprehn]

Created attachment 191072 [details]
Patch

[Adam Klein]

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...

[Elliott Sprehn]

Created attachment 191079 [details]
Patch

[Elliott Sprehn]

(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.

[Adam Barth]

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...

[Elliott Sprehn]

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.

[Adam Barth]

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".  :)

[WebKit Review Bot]

Comment on attachment 191079 [details]
Patch

Clearing flags on attachment: 191079

Committed r144522: <http://trac.webkit.org/changeset/144522>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.