Bug 66030 – [Chromium] New windows opened in javascript are named "_blank"

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

[Shinya Kawanaka]

A window created in javascript like window.open('', '_blank') has name '_blank'. The name of the window should be empty.

http://code.google.com/p/chromium/issues/detail?id=88129

[Shinya Kawanaka]

This is Chromium bug.
http://code.google.com/p/chromium/issues/detail?id=88129

[Shinya Kawanaka]

Created attachment 103583 [details]
Patch

[Nate Chapin]

Comment on attachment 103583 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=103583&action=review

> Source/WebCore/ChangeLog:12
> +
> +        No new tests because this bug occurs only in Chromium.
> +

Please include a test if at all possible.  Even port-specific bugs typically receive a new layout test, since other ports might regress this behavior at some point :)

> Source/WebCore/loader/FrameLoader.cpp:3265
> +    if (request.frameName() == "_blank")
> +        requestWithReferrer.setFrameName("");

This code isn't chromium-specific, so it seems odd that we're fixing this bug here.  How do the other ports avoid reaching this point with frame mame of "_blank"?  Is this a v8/jsc difference?

It's possible this is the right place to fix the bug, but it would be good to understand what chromium is doing differently before committing to this solution.

[Shinya Kawanaka]

Created attachment 103904 [details]
Patch

[Shinya Kawanaka]

Created attachment 103906 [details]
Layout Test

[Shinya Kawanaka]

(In reply to comment #5)
> Created an attachment (id=103906) [details]
> Layout Test

This bug occurs only in chromium.

I don't know the reason but it did not occur in layout tests (even in chromium port...)
So I would like to add tests on chromium side if permitted.

[Nate Chapin]

(In reply to comment #6)
> (In reply to comment #5)
> > Created an attachment (id=103906) [details] [details]
> > Layout Test
> 
> This bug occurs only in chromium.
> 
> I don't know the reason but it did not occur in layout tests (even in chromium port...)
> So I would like to add tests on chromium side if permitted.

So what has me curious is: why doesn't the check at http://trac.webkit.org/browser/trunk/Source/WebCore/loader/FrameLoader.cpp#L2854 prevent the frame name getting set to "_blank"?

I think it might be because of http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/content/renderer/render_view.cc&exact_package=chromium&l=403 .  Perhaps we should see if that fixes this issue, and whether it breaks anything else?

[Darin Fisher (:fishd, Google)]

(In reply to comment #7)
> I think it might be because of http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/content/renderer/render_view.cc&exact_package=chromium&l=403 .  Perhaps we should see if that fixes this issue, and whether it breaks anything else?

Oh, are you suggesting that maybe we should just delete that line of code in render_view.cc?  That makes sense since TestShell and DRT don't have any code like that.  Yeah, I bet you are correct that we could just remove that line of code.

[Nate Chapin]

(In reply to comment #8)
> (In reply to comment #7)
> > I think it might be because of http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/content/renderer/render_view.cc&exact_package=chromium&l=403 .  Perhaps we should see if that fixes this issue, and whether it breaks anything else?
> 
> Oh, are you suggesting that maybe we should just delete that line of code in render_view.cc?  That makes sense since TestShell and DRT don't have any code like that.  Yeah, I bet you are correct that we could just remove that line of code.

Yeah, that is what I was suggesting, sorry if it wasn't sufficiently clear.

I don't know whether that'll cause any problems for chromium, but it feels to me like render_view.cc is reaching into things that it shouldn't.

[Shinya Kawanaka]

Sorry if I miss the point.
Do you mean removing the line from render_view.cc after adding this patch?

If not so, could you inform me more precise direction? I would like to do that.

(In reply to comment #9)
> (In reply to comment #8)
> > (In reply to comment #7)
> > > I think it might be because of http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/content/renderer/render_view.cc&exact_package=chromium&l=403 .  Perhaps we should see if that fixes this issue, and whether it breaks anything else?
> > 
> > Oh, are you suggesting that maybe we should just delete that line of code in render_view.cc?  That makes sense since TestShell and DRT don't have any code like that.  Yeah, I bet you are correct that we could just remove that line of code.
> 
> Yeah, that is what I was suggesting, sorry if it wasn't sufficiently clear.
> 
> I don't know whether that'll cause any problems for chromium, but it feels to me like render_view.cc is reaching into things that it shouldn't.

[Nate Chapin]

(In reply to comment #10)
> Sorry if I miss the point.
> Do you mean removing the line from render_view.cc after adding this patch?
> 
> If not so, could you inform me more precise direction? I would like to do that.
> 

I was thinking that just removing those 2 lines from render_view.cc (and not modifying ChromeClientImpl.cpp) might fix the problem.  (I could be wrong, of course) :)

[Darin Fisher (:fishd, Google)]

I suspect that Nate is correct.  That might be all it takes to fix this bug.

[Darin Fisher (:fishd, Google)]

Can we close this bug given that the solution is happening on the Chromium side?

[Shinya Kawanaka]

(In reply to comment #13)
> Can we close this bug given that the solution is happening on the Chromium side?

I think so.