Bug 32383 - Selecting article text at sfgate.com results in spurious back/forward entries
Summary: Selecting article text at sfgate.com results in spurious back/forward entries
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: History (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Brady Eidson
URL: http://www.sfgate.com
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2009-12-10 10:52 PST by Brady Eidson
Modified: 2009-12-24 16:42 PST (History)
4 users (show)

See Also:


Attachments
Dynamically created iframe with automatic form submission (Our goal is to match both IE and Firefox and make this *not* add a back entry) (929 bytes, text/html)
2009-12-10 20:29 PST, Brady Eidson
no flags Details
Parsed <iframe> with dynamically added form - Firefox and IE differ here (706 bytes, text/html)
2009-12-10 20:29 PST, Brady Eidson
no flags Details
Proposed fix, new layout tests, and updated results for 1 old test. (33.39 KB, patch)
2009-12-10 21:45 PST, Brady Eidson
no flags Details | Formatted Diff | Diff
Much simpler rule, much simpler patch (14.51 KB, patch)
2009-12-11 15:45 PST, Brady Eidson
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brady Eidson 2009-12-10 10:52:45 PST
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>
Comment 1 Brady Eidson 2009-12-10 15:16:54 PST
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.
Comment 2 Brady Eidson 2009-12-10 20:27:34 PST
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.
Comment 3 Brady Eidson 2009-12-10 20:29:15 PST
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)
Comment 4 Brady Eidson 2009-12-10 20:29:54 PST
Created attachment 44660 [details]
Parsed <iframe> with dynamically added form - Firefox and IE differ here
Comment 5 Brady Eidson 2009-12-10 21:21:40 PST
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.
Comment 6 Brady Eidson 2009-12-10 21:31:58 PST
FWIW, Opera matches IE behavior here - they do not add an extra item in either case.
Comment 7 Brady Eidson 2009-12-10 21:33:53 PST
Note that r25410 was a fix for https://bugs.webkit.org/show_bug.cgi?id=14957
Comment 8 Brady Eidson 2009-12-10 21:43:55 PST
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.
Comment 9 Brady Eidson 2009-12-10 21:45:36 PST
Created attachment 44661 [details]
Proposed fix, new layout tests, and updated results for 1 old test.
Comment 10 WebKit Review Bot 2009-12-10 21:49:13 PST
style-queue ran check-webkit-style on attachment 44661 [details] without any errors.
Comment 11 Darin Adler 2009-12-11 07:27:53 PST
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?
Comment 12 Brady Eidson 2009-12-11 09:32:17 PST
"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.
Comment 13 Brady Eidson 2009-12-11 12:53:40 PST
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.
Comment 14 Brady Eidson 2009-12-11 14:40:48 PST
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
Comment 15 Brady Eidson 2009-12-11 15:18:50 PST
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.
Comment 16 Brady Eidson 2009-12-11 15:45:53 PST
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.
Comment 17 Eric Seidel (no email) 2009-12-11 15:48:52 PST
I'm slightly confused as to why this is marked as a sensitive (hidden) bug?
Comment 18 Brady Eidson 2009-12-11 15:51:45 PST
Due to some accident...
Comment 19 Brady Eidson 2009-12-11 15:53:09 PST
beidson@apple.com	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.
Comment 20 WebKit Review Bot 2009-12-11 15:57:02 PST
style-queue ran check-webkit-style on attachment 44715 [details] without any errors.
Comment 21 Darin Adler 2009-12-11 16:20:55 PST
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
Comment 22 Brady Eidson 2009-12-11 17:35:11 PST
Ran the suggested changes by Darin, and landed in http://trac.webkit.org/changeset/52033
Comment 23 Nikolas Zimmermann 2009-12-24 16:42:27 PST
(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?