Bug 32383 – Selecting article text at sfgate.com results in spurious back/forward entries

• Status: RESOLVED
• Resolution: FIXED
• Component: History

[Brady Eidson]

Selecting article text at sfgate.com results in spurious back/forward entries.

Steps:
- Visit http://www.sfgate.com
- Click any article
- Click and hold back button, note no extraneous entries in back/forward list
- Select portions of the article a few times.
- Notice each time you do this, Safari seems to go through a page load (based on the loading indicator in the location field)
- Click and hold back button again, note extraneous entries in back/forward list
- Back no longer takes you out of the article, due to all the extra entries

What's going on is that sfgate.com is doing some sort of tracking on mouse up after a selection by doing the following:
-Creating an iframe element with some forms
-Appending the iframe to the DOM
-Submitting the form
...then later:
-Removing the iframe from the DOM

The current heuristics WebKit has to not add a back/forward entry don't preclude this iframe form submission, so an entry gets added for the "navigation"

Without too much difficult, when a form is programmatically submitted we can note that:
A - The frame was *not* created by the parser - that is, it was dynamically created by script.
B - The form submission was not the result of a user event - that is, the submission was also done by script.

In this case, we can lock the back/forward list.

I can't think of a reasonable site this would break, and will know if this approach passes layout tests shortly.  At which point I'll post my patch.

This problem is in radar as <rdar://problem/7342725>

[Brady Eidson]

In further experimentation, it appears that Firefox implements this rule.  In testing:
1 - iframe created by the parser and submitted manually
2 - iframe created by the parser and submitted programmatically
3 - iframe created by script and submitted manually
4 - iframe created by script and submitted programmatically

#'s 1-3 add a back/forward item in Firefox, whereas #4 does not, and that's what I'll be going for here.

[Brady Eidson]

This story keeps getting more bizarre - It appears we're the only ones that get dynamic iframe creation right.

The IE documentation for the <iframe> element has people complaining it is buggy and not reliable in IE - http://msdn.microsoft.com/en-us/library/ms535258(VS.85).aspx

Firefox also has a pretty gnarly bug - https://bugzilla.mozilla.org/show_bug.cgi?id=473396 "Firefox does not draw appended contents of DOM created empty iframe."

In both Firefox and IE have almost exactly the same bug.  If you dynamically create and iframe and dynamically put some crap in it, then that crap doesn't render.  I will attach an example of this shortly called "dynamic-auto.html".

Additionally, if that dynamically created iframe has a form and script submits it, the submission takes place and you do NOT get a back/forward entry.  This is true in both IE and FIrefox.

I will attach an example called "parsed-auto.html".  In this example, an empty <iframe> element exists in the markup itself.  The only dynamic part of it is creating the form elements and adding them to the iframe, then submitting the form.  This works well in all 3.

These two tests give us the following matrix:

IE8:
Parsed iFrame, dynamic form and submission - NO BACK ENTRY
Dynamic iframe, dynamic form and submission - NO BACK ENTRY (along with other fun IE bugs)

Firefox:
Parsed iFrame, dynamic form and submission - BACK ENTRY ADDED
Dynamic iframe, dynamic form and submission - NO BACK ENTRY (along with other fun Firefox bugs)

WebKit:
Parsed iFrame, dynamic form and submission - BACK ENTRY ADDED
Dynamic iframe, dynamic form and submission - BACK ENTRY ADDED

If we changed to match both IE and Firefox in the Dynamically created iFrame case, we would fix this bug.  There would still be a difference between us and IE for the parsed <iframe> case but until I see that affect a real world site, I suggest we leave our behavior as-is.

[Brady Eidson]

Created attachment 44659 [details]
Dynamically created iframe with automatic form submission (Our goal is to match both IE and Firefox and make this *not* add a back entry)

[Brady Eidson]

Created attachment 44660 [details]
Parsed <iframe> with dynamically added form - Firefox and IE differ here

[Brady Eidson]

My upcoming patch causes 1 layout test failure, http/tests/navigation/onload-navigation-iframe-timeout.html

That test was added in http://trac.webkit.org/changeset/25410 which - amazingly - was fixing "spurious back button entries at sfgate.com"

The test states "In this case, Safari and Firefox adds a history item but IE doesn't" and the change in behavior is now we're *not* adding an item, and acting like IE.

I would argue that based on my exploration of this issue, the fact that my patch fixes sfgate.com now, and the previous sfgate issue remains fixed, that this layout test change is a progression.

[Brady Eidson]

FWIW, Opera matches IE behavior here - they do not add an extra item in either case.

[Brady Eidson]

Note that r25410 was a fix for https://bugs.webkit.org/show_bug.cgi?id=14957

[Brady Eidson]

If the patch I am about to submit is what lands to resolve this bug, then https://bugs.webkit.org/show_bug.cgi?id=22851 should be resolved also.

[Brady Eidson]

Created attachment 44661 [details]
Proposed fix, new layout tests, and updated results for 1 old test.

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 44661 [details] without any errors.

[Darin Adler]

Comment on attachment 44661 [details]
Proposed fix, new layout tests, and updated results for 1 old test.

I'm not sure if "created by parser and submitted by JavaScript" is the right recipe here. For example, you can create frames using innerHTML and that counts as created by parser. Is the "created by parser and submitted by JavaScript" rule really what the other browsers are doing? How did you test that?

[Brady Eidson]

"created by parser and submitted by JavaScript" is - as far as my testing goes so far - COMPATIBLE with other browsers are doing.

I can't tell you with certainty that it is exactly what other browsers are doing because - IE and Firefox at least - both seem to have pretty nutty bugs with iframes created via document.createNode()

I will try your innerHTML case and see if I learn anything else.

[Brady Eidson]

Darin has convinced me to match IE/Opera directly and never add back/forward entries for dynamic form submissions in iframes no matter how the iframe got there.

[Brady Eidson]

Changing to match my best understanding of IE8/Opera causes a series of layouttest regressions.

Failed results:
fast/forms/button-state-restore.html
http/tests/history/back-to-post.php	
http/tests/history/redirect-js-form-submit-0-seconds.html	
http/tests/history/redirect-js-form-submit-2-seconds.html	
http/tests/history/redirect-js-form-submit-before-load.html	
http/tests/navigation/back-send-referrer.html	

Timed out:
http/tests/cache/subresource-failover-to-network.html

[Brady Eidson]

Those failures reflect my patch implementing the wrong rule.

I meant to implement "when forms in iframes are submitted by script, lock the back/forward list."
What I implemented was "when ANY form in ANY frame is submitted by script, lock the back/forward list."

After implementing the correct rule I get NO change in behavior on existing layout tests.

[Brady Eidson]

Created attachment 44715 [details]
Much simpler rule, much simpler patch

This implements the rule "if a form submission was done by javascript and is in an iframe, lock the back/forward list"

It matches IE and Opera as best as I can figure, and doesn't change any other layout tests in the process.

[Eric Seidel (no email)]

I'm slightly confused as to why this is marked as a sensitive (hidden) bug?

[Brady Eidson]

Due to some accident...

[Brady Eidson]

[e-mail]	2009-12-11 15:18:50 PST	 Group		Security-Sensitive


And I hadn't made any changes immediately before or after that.  I wonder how on earth I accidentally did that without meaning to.  heh.

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 44715 [details] without any errors.

[Darin Adler]

Comment on attachment 44715 [details]
Much simpler rule, much simpler patch

Brady and I discussed some refinements:

    1) Using an enum instead of a boolean.
    2) Reducing the number of arguments to the public versions at least, of the submit function on HTMLFormElement.

I think this is OK to land as-is, but if you want to change to an enum that's cool too.

r=me

[Brady Eidson]

Ran the suggested changes by Darin, and landed in http://trac.webkit.org/changeset/52033

[Nikolas Zimmermann]

(In reply to comment #22)
> Ran the suggested changes by Darin, and landed in
> http://trac.webkit.org/changeset/52033

In case it hasn't been noticed so far: since the patch landed, the two new tests *dynamic-iframe-dynamic-form-back-entry.html, etc.) fail on the tiger build slaves. Does anyone plan to fix this or does this need a roll-out?