Bug 32383 - Selecting article text at sfgate.com results in spurious back/forward entries
: Selecting article text at sfgate.com results in spurious back/forward entries
Status: RESOLVED FIXED
: WebKit
History
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To:
: http://www.sfgate.com
: InRadar
:
:
  Show dependency treegraph
 
Reported: 2009-12-10 10:52 PST by
Modified: 2009-12-24 16:42 PST (History)


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 Review Patch | Details | Formatted Diff | Diff
Much simpler rule, much simpler patch (14.51 KB, patch)
2009-12-11 15:45 PST, Brady Eidson
darin: review+
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 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 From 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 From 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 From 2009-12-10 20:29:15 PST -------
Created an attachment (id=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 From 2009-12-10 20:29:54 PST -------
Created an attachment (id=44660) [details]
Parsed <iframe> with dynamically added form - Firefox and IE differ here
------- Comment #5 From 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 From 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 From 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 From 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 From 2009-12-10 21:45:36 PST -------
Created an attachment (id=44661) [details]
Proposed fix, new layout tests, and updated results for 1 old test.
------- Comment #10 From 2009-12-10 21:49:13 PST -------
style-queue ran check-webkit-style on attachment 44661 [details] without any errors.
------- Comment #11 From 2009-12-11 07:27:53 PST -------
(From update of attachment 44661 [details])
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 From 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 From 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 From 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 From 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 From 2009-12-11 15:45:53 PST -------
Created an attachment (id=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 From 2009-12-11 15:48:52 PST -------
I'm slightly confused as to why this is marked as a sensitive (hidden) bug?
------- Comment #18 From 2009-12-11 15:51:45 PST -------
Due to some accident...
------- Comment #19 From 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 From 2009-12-11 15:57:02 PST -------
style-queue ran check-webkit-style on attachment 44715 [details] without any errors.
------- Comment #21 From 2009-12-11 16:20:55 PST -------
(From update of attachment 44715 [details])
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 From 2009-12-11 17:35:11 PST -------
Ran the suggested changes by Darin, and landed in http://trac.webkit.org/changeset/52033
------- Comment #23 From 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?