<webview>: Workaround to enable webview.contentWindow.postMessage.

<webview>: Workaround to enable webview.contentWindow.postMessage.
Test=manually

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=172507

[sadrul, 2012-12-11 01:48:55]

https://codereview.chromium.org/11515016/diff/1/chrome/renderer/resources/ext...
File chrome/renderer/resources/extensions/web_view.js (right):

https://codereview.chromium.org/11515016/diff/1/chrome/renderer/resources/ext...
chrome/renderer/resources/extensions/web_view.js:105: return
objectNode.contentWindow.frames;
Hm. I don't know enough about contentWindow to know what this really means.
Perhaps someone who knows JS could review instead?

[Fady Samuel, 2012-12-11 01:53:08]

Hi Istiaque,

Are you comfortable reviewing this?

Thanks,

Fady

[lazyboy, 2012-12-11 16:20:15]

Just one concern, the contentWindow.frames magic looks good.

https://codereview.chromium.org/11515016/diff/1/chrome/renderer/resources/ext...
File chrome/renderer/resources/extensions/web_view.js (right):

https://codereview.chromium.org/11515016/diff/1/chrome/renderer/resources/ext...
chrome/renderer/resources/extensions/web_view.js:101:
Object.defineProperty(this.node_, 'contentWindow', {
Hopefully this doesn't break other properties with contentWindow.
What else (other than postMessage) can we do with <webview>.contentWindow? Can
we check if they still work properly?

[Fady Samuel, 2012-12-11 20:45:37]

Hi Nasko,

This is a workaround for a V8/WebKit bug. I was trying to check if I can access
the contentWindow's name, or frame tree, but apparently not. Is this disabled on
swapped out RenderViews? We don't keep it in sync with the real RenderView
right? Thanks.

[(Do not use) nasko, 2012-12-11 21:13:13]

No. We disabled the frame tree updates, due to the race condition you
found. We will redo the work once we have more clarity of how we will
structure out-of-process iframes.

On Tue, Dec 11, 2012 at 12:45 PM, <fsamuel@chromium.org> wrote:

> Hi Nasko,
>
> This is a workaround for a V8/WebKit bug. I was trying to check if I can
> access
> the contentWindow's name, or frame tree, but apparently not. Is this
> disabled on
> swapped out RenderViews? We don't keep it in sync with the real RenderView
> right? Thanks.
>
>
https://codereview.chromium.**org/11515016/<https://codereview.chromium.org/1...
>

[Fady Samuel, 2012-12-11 21:15:16]

Thanks Nasko! Sadrul could you please rubberstamp this based on the comments
above? Thanks!

[Fady Samuel, 2012-12-11 21:17:30]

Actually, running this by Charlie first. Any objections? Thanks!

[Charlie Reis, 2012-12-11 21:30:12]

https://codereview.chromium.org/11515016/diff/1/chrome/renderer/resources/ext...
File chrome/renderer/resources/extensions/web_view.js (right):

https://codereview.chromium.org/11515016/diff/1/chrome/renderer/resources/ext...
chrome/renderer/resources/extensions/web_view.js:101:
Object.defineProperty(this.node_, 'contentWindow', {
I'm a bit nervous about this, but maybe it's the right thing to do.

Can you explain more about the original bug?  What is contentWindow actually
pointing to, and why is contentWindow.frames the right answer?

At least for the time being, I don't think we support any other uses of
contentWindow than postMessage (e.g., no openers, no traversal of frame trees,
no close(), focus(), etc), so it might not be a big problem.  I want to
understand why it fixes the bug, though, to be sure.

[Fady Samuel, 2012-12-11 21:46:01]

https://codereview.chromium.org/11515016/diff/1/chrome/renderer/resources/ext...
File chrome/renderer/resources/extensions/web_view.js (right):

https://codereview.chromium.org/11515016/diff/1/chrome/renderer/resources/ext...
chrome/renderer/resources/extensions/web_view.js:101:
Object.defineProperty(this.node_, 'contentWindow', {
On 2012/12/11 21:30:12, creis wrote:
> I'm a bit nervous about this, but maybe it's the right thing to do.
> 
> Can you explain more about the original bug?  What is contentWindow actually
> pointing to, and why is contentWindow.frames the right answer?
> 
> At least for the time being, I don't think we support any other uses of
> contentWindow than postMessage (e.g., no openers, no traversal of frame trees,
> no close(), focus(), etc), so it might not be a big problem.  I want to
> understand why it fixes the bug, though, to be sure.

My understanding of the WebKit bug is we're creating an object wrapper around
the the v8 window object of the swapped out RenderView.

When we call browserPlugin.contentWindow.postMessage it seems like it's being
called in the context of the wrapper instead of the context of the window
object. Thus, postMessage fails. browserPlugin.contentWindow.frames escapes this
wrapper and so we get the window object and the correct context for postMessage.

For example console.log(browserPlugin.contentWindow) returns 'Object' as the
type, whereas console.log(browserPlugin.contentWindow.frames) returns 'Window'.

Actually, it looks like browserPlugin.contentWindow.self also escapes the
wrapper if that's preferable.

[Charlie Reis, 2012-12-11 22:00:09]

On 2012/12/11 21:46:01, Fady Samuel wrote:
>
https://codereview.chromium.org/11515016/diff/1/chrome/renderer/resources/ext...
> File chrome/renderer/resources/extensions/web_view.js (right):
> 
>
https://codereview.chromium.org/11515016/diff/1/chrome/renderer/resources/ext...
> chrome/renderer/resources/extensions/web_view.js:101:
> Object.defineProperty(this.node_, 'contentWindow', {
> On 2012/12/11 21:30:12, creis wrote:
> > I'm a bit nervous about this, but maybe it's the right thing to do.
> > 
> > Can you explain more about the original bug?  What is contentWindow actually
> > pointing to, and why is contentWindow.frames the right answer?
> > 
> > At least for the time being, I don't think we support any other uses of
> > contentWindow than postMessage (e.g., no openers, no traversal of frame
trees,
> > no close(), focus(), etc), so it might not be a big problem.  I want to
> > understand why it fixes the bug, though, to be sure.
> 
> My understanding of the WebKit bug is we're creating an object wrapper around
> the the v8 window object of the swapped out RenderView.
> 
> When we call browserPlugin.contentWindow.postMessage it seems like it's being
> called in the context of the wrapper instead of the context of the window
> object. Thus, postMessage fails. browserPlugin.contentWindow.frames escapes
this
> wrapper and so we get the window object and the correct context for
postMessage.
> 
> For example console.log(browserPlugin.contentWindow) returns 'Object' as the
> type, whereas console.log(browserPlugin.contentWindow.frames) returns
'Window'.
> 
> Actually, it looks like browserPlugin.contentWindow.self also escapes the
> wrapper if that's preferable.

Yes, let's do contentWindow.self.  And that makes it sound like we're doing the
right thing.  LGTM with that change.

[commit-bot: I haz the power, 2012-12-11 22:27:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fsamuel@chromium.org/11515016/11001

[commit-bot: I haz the power, 2012-12-12 02:51:43]

Change committed as 172507