Stop using an iframe's id as fallback if its name attribute is not set

[Madhu M]

For iframe if the name is not specified, id is assigned to name.If both are not specified, name is getting assigned with self.name::<!--framePath //<!--frame0-->-->. If name(if not id) is given as _blank, it is getting the value as "_blank".

In Firefox if name is undefined or null then it will be shown as empty string.It will not take id as name, if id alone is given.If name is _blank, it will not get changed to ""

[Alexey Proskuryakov]

We will likely need separate bugs for each of these issues. Please note that at least some of them are already known, e.g. bug 6751.

[Madhu M]

Created attachment 11207 [details]
Patch for review for the iframe name issue

I have tried to fix this issue and make the behaviour similar to Firefox. All these issues are related.I have commented the code which is generating the unique name if the name of the frame is null or undefined. It will take the name as empty string in this case. If the name is not given and id is present, then also the name will be taken as "" instead of id

[Madhu M]

Created attachment 11208 [details]
Zip file containing samples to simulate the bug

The attached ZIP is having sample files to show the bug. It is behaving differently in Safari and Firefox. After making the code changes as in the patch attached, it gives the same behaviour in both the browsers.

[Madhu M]

Please review the fix

[Madhu M]

Please review this bug and give comments about the fix

[Maciej Stachowiak]

Comment on attachment 11207 [details]
Patch for review for the iframe name issue

Thanks for the patch! It needs a couple of problems fixed to be landable.

1) This patch appears to be formatted as an RTF file or something (lots of weird backslashes). We need a plaintext patch to be able to land it.

2) The patch should not leave in commented code - if some lines of code are wrong, you should just remove them. There's also no need to put comments explaining why certain things aren't done.

3) The patch should include test cases in the form of tests and expected results for the LayoutTests directory, since it changes behavior for web content.  The samples should be part of the patch, not just a separate zip file.

4) I am pretty sure the unique frame names are for a reason, specifically for the back/forward list to be able to find things properly when you do back/forward in nested framesets. Did you test this case?

5) The patch needs to include a ChangeLog entry.

Thanks for the submission, please address these comments and submit a new patch. r- for now since this needs revision.

[Mark Rowe (bdash)]

Madhu, it'd be great if you could address Maciej's comments and resubmit this patch!

[Alexey Proskuryakov]

*** Bug 69236 has been marked as a duplicate of this bug. ***

[Sameer Patil]

Created attachment 109772 [details]
Proposed patch for frame and iframe incorrect name assignment

This issue looks generic and applies to frames too. Uploading proposed patch which solve this issue for frame and iframe. Uniquechildname issue adressed by Madhu in previous patch is fixed, please check https://bugs.webkit.org/show_bug.cgi?id=6751.I have modified LayoutTest/fast/frames/frame-element-name.html to incorporate test case related this issue.

[WebKit Review Bot]

Comment on attachment 109772 [details]
Proposed patch for frame and iframe incorrect name assignment

Attachment 109772 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9959171

New failing tests:
http/tests/multipart/multipart-wait-before-boundary.html
http/tests/security/cross-frame-access-document-direct.html
http/tests/security/cross-frame-access-parent-explicit-domain.html
http/tests/security/cross-frame-access-port-explicit-domain.html
fast/dom/Window/slow-unload-handler.html
http/tests/navigation/no-referrer-subframe.html
http/tests/loading/redirect-methods.html
fast/events/open-window-from-another-frame.html
fast/events/onbeforeunload-focused-iframe.html
fast/dom/wrapper-context.html
http/tests/navigation/forward-and-cancel.html
http/tests/security/cross-frame-access-protocol-explicit-domain.html
http/tests/security/cross-frame-access-port.html
fast/dom/xss-DENIED-javascript-variations.html
http/tests/security/cross-frame-access-protocol.html
fast/dom/Window/window-special-properties.html
http/tests/misc/location-replace-crossdomain.html
fast/dom/Window/slow-unload-handler-only-frame-is-stopped.html
http/tests/navigation/image-load-in-subframe-unload-handler.html
http/tests/security/cross-frame-access-child-explicit-domain.html

[Adam Barth]

Does Firefox still differ from our behavior here?  Does the HTML5 spec have a preference between the two behaviors?  What about IE?

[Boris Zbarsky]

(In reply to comment #11)
> Does Firefox still differ from our behavior here?

Yes.

> Does the HTML5 spec have a preference between the two behaviors?

Yes.  See bug 69236 which has all this information.

[Sameer Patil]

(In reply to comment #11)
>What about IE?
IE9 matches the behavior with Firefox.

[Ryosuke Niwa]

Comment on attachment 109772 [details]
Proposed patch for frame and iframe incorrect name assignment

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

> Source/WebCore/ChangeLog:8
> +        Frame name getting assigned from id attribute if it's value is null.

Please refer to a relevant spec.

> Source/WebCore/ChangeLog:9
> +

Please mention that you've added a test case to fast/frames/frame-element-name.html

[Sameer Patil]

Created attachment 125812 [details]
Updated patch

Uploading updated patch, incorporating review comments.

[Sameer Patil]

Created attachment 126754 [details]
Updated patch

Re-uploading patch as previous patch is not applied properly.

[Sameer Patil]

Created attachment 127124 [details]
Updated patch

[WebKit Review Bot]

Comment on attachment 127124 [details]
Updated patch

Attachment 127124 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11521620

New failing tests:
http/tests/multipart/multipart-wait-before-boundary.html
http/tests/navigation/forward-and-cancel.html
http/tests/misc/location-replace-crossdomain.html
fast/autoresize/autoresize-with-iframe.html
http/tests/security/dataURL/xss-DENIED-from-data-url-to-data-url.html
http/tests/security/cross-frame-access-protocol.html
http/tests/loading/redirect-methods.html
http/tests/security/cross-frame-access-document-direct.html
http/tests/inspector/inspect-element.html
http/tests/security/dataURL/xss-DENIED-to-data-url-sub-frame-uppercase.html
http/tests/security/protocol-compare-case-insensitive.html
http/tests/security/dataURL/xss-DENIED-to-data-url-from-data-url.html
http/tests/navigation/no-referrer-subframe.html
http/tests/security/cross-frame-access-child-explicit-domain.html
http/tests/security/cross-frame-access-port-explicit-domain.html
http/tests/security/host-compare-case-insensitive.html
http/tests/security/dataURL/xss-DENIED-to-data-url-sub-frame.html
http/tests/security/dataURL/xss-DENIED-from-data-url-sub-frame-2-level.html
http/tests/navigation/image-load-in-subframe-unload-handler.html
http/tests/security/cross-frame-access-port.html
http/tests/security/dataURL/xss-DENIED-from-data-url-sub-frame.html
accessibility/aria-describedby-on-input.html
http/tests/security/cross-frame-access-protocol-explicit-domain.html
http/tests/security/dataURL/xss-DENIED-to-data-url-sub-frame-2-level.html
http/tests/security/dataURL/xss-DENIED-to-data-url-in-foreign-domain-subframe.html
http/tests/security/cross-frame-access-parent-explicit-domain.html
http/tests/security/dataURL/xss-DENIED-from-data-url-in-foreign-domain-subframe.html
fast/dom/Window/slow-unload-handler.html
fast/dom/Window/timeout-callback-scope.html
fast/dom/Window/slow-unload-handler-only-frame-is-stopped.html

[WebKit Review Bot]

Attachment 127124 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1

Tools/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Total errors found: 1 in 756 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Sameer Patil]

Created attachment 127155 [details]
Updated patch

Uploading latest patch, fixing failing Layout tests.

[WebKit Review Bot]

Comment on attachment 127155 [details]
Updated patch

Attachment 127155 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11514857

New failing tests:
http/tests/security/javascriptURL/javascriptURL-execution-context-iframe-src-setAttributeNS.html
http/tests/navigation/forward-and-cancel.html
http/tests/misc/location-replace-crossdomain.html
http/tests/security/javascriptURL/javascriptURL-execution-context-iframe-src-getAttribute-value.html
http/tests/security/javascriptURL/javascriptURL-execution-context-frame-src-setAttributeNS.html
http/tests/security/javascriptURL/javascriptURL-execution-context-frame-src-getAttribute-value.html
http/tests/security/javascriptURL/javascriptURL-execution-context-frame-src-htmldom.html
http/tests/inspector/inspect-element.html
http/tests/security/javascriptURL/javascriptURL-execution-context-frame-location-htmldom.html
http/tests/security/xssAuditor/full-block-base-href.html
http/tests/security/javascriptURL/javascriptURL-execution-context-frame-src-setAttributeNodeNS.html
http/tests/security/javascriptURL/javascriptURL-execution-context-iframe-src-setAttributeNodeNS.html
http/tests/security/javascriptURL/xss-DENIED-from-javascript-url-in-foreign-domain-subframe.html
http/tests/security/javascriptURL/javascriptURL-execution-context-frame-src-setAttributeNode.html
http/tests/security/javascriptURL/javascriptURL-execution-context-iframe-src-setAttributeNode.html
http/tests/security/javascriptURL/javascriptURL-execution-context-iframe-src-setAttribute.html
http/tests/security/javascriptURL/xss-ALLOWED-from-javascript-url-sub-frame.html
http/tests/security/dataURL/xss-DENIED-from-data-url-to-data-url.html
accessibility/aria-describedby-on-input.html
http/tests/security/javascriptURL/javascriptURL-execution-context-iframe-src-htmldom.html
fast/dom/Window/window-collection-length-no-crash.html
http/tests/security/javascriptURL/javascriptURL-execution-context-frame-src-setAttribute.html
http/tests/security/dataURL/xss-DENIED-from-data-url-in-foreign-domain-subframe.html
http/tests/security/javascriptURL/xss-ALLOWED-to-javascript-url-from-javscript-url.html
fast/dom/wrapper-context.html
fast/dom/Window/window-access-after-navigation.html
http/tests/security/javascriptURL/xss-ALLOWED-to-javascript-url-sub-frame.html
http/tests/security/javascriptURL/xss-ALLOWED-from-javascript-url-sub-frame-2-level.html
http/tests/security/javascriptURL/xss-DENIED-to-javascript-url-in-foreign-domain-subframe.html
http/tests/security/javascriptURL/xss-ALLOWED-to-javascript-url-sub-frame-2-level.html

[Sameer Patil]

Created attachment 127529 [details]
Updated patch

[WebKit Review Bot]

Comment on attachment 127529 [details]
Updated patch

Attachment 127529 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11541374

New failing tests:
platform/chromium/plugins/get-url-with-iframe-target-no-crash.html
fast/events/popup-when-select-change.html
fast/events/popup-blocked-to-post-blank.html
fast/dom/wrapper-context.html
http/tests/navigation/forward-and-cancel.html
fast/dom/Window/window-access-after-navigation.html
http/tests/misc/location-replace-crossdomain.html
plugins/get-url-with-iframe-target.html
fast/dom/Window/window-special-properties.html
http/tests/security/dataURL/xss-DENIED-from-data-url-to-data-url.html
fast/overflow/scrollRevealButton.html
fullscreen/full-screen-frameset.html
fast/dom/Window/window-collection-length-no-crash.html
fast/history/saves-state-after-frame-nav.html

[Chris Dumez]

*** Bug 185243 has been marked as a duplicate of this bug. ***

[Chris Dumez]

Created attachment 339440 [details]
Patch

[Chris Dumez]

Created attachment 339448 [details]
Patch

[Chris Dumez]

Created attachment 339461 [details]
Patch

[Geoffrey Garen]

Comment on attachment 339461 [details]
Patch

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

r=me

> Source/WebCore/ChangeLog:17
> +        This WebKit quirk was causing some Web-compatibility issue because it

issues

[EWS Watchlist]

Comment on attachment 339461 [details]
Patch

Attachment 339461 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/7556037

New failing tests:
http/tests/quicklook/csp-header-ignored.html

[EWS Watchlist]

Created attachment 339477 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.4

[Chris Dumez]

Created attachment 339478 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 339478 [details]
Patch

Clearing flags on attachment: 339478

Committed r231331: <https://trac.webkit.org/changeset/231331>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/39959117>

[Chris Dumez]

Reverted r231331 for reason:

Caused a few tests to assert

Committed r231367: <https://trac.webkit.org/changeset/231367>

[Chris Dumez]

Created attachment 339738 [details]
Patch

[EWS Watchlist]

Comment on attachment 339738 [details]
Patch

Attachment 339738 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/7597465

New failing tests:
fast/dom/Geolocation/srcdoc-getCurrentPosition.html
http/tests/navigation/image-load-in-subframe-unload-handler.html
fast/dom/Geolocation/srcdoc-watchPosition.html
http/tests/loading/basic-auth-load-URL-with-consecutive-slashes.html
http/tests/security/contentSecurityPolicy/iframe-blank-url-programmatically-add-external-script.html
fast/xmlhttprequest/xmlhttprequest-no-file-access.html

[EWS Watchlist]

Created attachment 339743 [details]
Archive of layout-test-results from ews101 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Chris Dumez]

Created attachment 339744 [details]
Patch

[EWS Watchlist]

Comment on attachment 339744 [details]
Patch

Attachment 339744 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/7598026

New failing tests:
http/tests/navigation/image-load-in-subframe-unload-handler.html

[EWS Watchlist]

Created attachment 339750 [details]
Archive of layout-test-results from ews102 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Chris Dumez]

Created attachment 339751 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 339751 [details]
Patch

Clearing flags on attachment: 339751

Committed r231456: <https://trac.webkit.org/changeset/231456>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

(In reply to WebKit Commit Bot from comment #43)
> Comment on attachment 339751 [details]
> Patch
> 
> Clearing flags on attachment: 339751
> 
> Committed r231456: <https://trac.webkit.org/changeset/231456>

This broke the macOS Colloquy app. See bug 186197.

[Daniel Jalkut]

This changes is also breaking MarsEdit's rich text editor. Bug 187937.