Bug 13686 - REGRESSION: Certain iframes embedded in page listed in History menu
Summary: REGRESSION: Certain iframes embedded in page listed in History menu
Status: RESOLVED WORKSFORME
Alias: None
Product: WebKit
Classification: Unclassified
Component: History (show other bugs)
Version: 523.x (Safari 3)
Hardware: Mac OS X 10.4
: P1 Normal
Assignee: Nobody
URL: http://wsj.com
Keywords: Regression
Depends on:
Blocks:
 
Reported: 2007-05-11 18:02 PDT by Geoffrey F. Green
Modified: 2007-07-18 15:27 PDT (History)
2 users (show)

See Also:


Attachments
Screenshot of History menu (132.21 KB, image/png)
2007-05-11 18:05 PDT, Geoffrey F. Green
no flags Details
Proposed patch (4.91 KB, patch)
2007-06-13 06:10 PDT, Maxime BRITTO
no flags Details | Formatted Diff | Diff
cleaned up patch (1.77 KB, patch)
2007-06-15 06:00 PDT, Maxime BRITTO
mjs: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Geoffrey F. Green 2007-05-11 18:02:58 PDT
This is related to bug 13465, the substance of which was fixed in change 21367. In bug 13465, the content of a typical page on the WSJ.com site were replaced by the contents of an advertising iframe after the first page had loaded in; therefore, the history menu would contain an entry for the WSJ.com page, and then a separate entry for the iframe that was improperly loaded in to replace the original page. Change 21367 fixed the bulk of the problem, so the iframe now renders within the page instead of replacing it, but the History menu still lists the iframe as a separate page (Kanoodle test 303 in the attachment below).

Steps to reproduce:

1) Visit wsj.com. Wait for page to load in completely.

2) View history menu

Actual result: Most recent entry in history menu is "Kanoodle test 303"; second entry is "U.S. Home - WSJ.com"

Expected result: Most recent entry in history menu is "U.S. Home - WSJ.com"

Build-date and platform: r21368 and r21420 (2006-05-11) Mac OS 10.4.9/Intel.

Doesen't occur on: Safari 2.0.4 (419.3)
Comment 1 Geoffrey F. Green 2007-05-11 18:05:24 PDT
Created attachment 14499 [details]
Screenshot of History menu
Comment 2 David Kilzer (:ddkilzer) 2007-05-12 23:26:45 PDT
Confirmed with a local debug build of WebKit r21427 with Safari 2.0.4 (419.3) on Mac OS X 10.4.9 (8P135).

Giving this bug the full regression treatment.

Comment 3 Maxime BRITTO 2007-06-13 06:10:00 PDT
Created attachment 14998 [details]
Proposed patch

I added a condtion before calling the addHistoryForCurrentLocation() method from updateHistoryForInternalLoad : the frame must be the main frame to be added.
It works on the wall street journal website and all the layout tests are ok.
Comment 4 Maxime BRITTO 2007-06-15 06:00:57 PDT
Created attachment 15051 [details]
cleaned up patch

Following the advices a reviewer gave me for another patch I submitted ; I cleaned up this one and added a brief description of the modifications.
Comment 5 Maciej Stachowiak 2007-06-25 20:11:52 PDT
Comment on attachment 15051 [details]
cleaned up patch

Fix looks good to me. Please add a layout test. I believe this could be tested using dumpBackForwardList().

The comment is subtly wrong:

+            // We only add the main frames to the history


We do, in fact, add loads of frames other than the main frame to the history, if they are not initial loads, but that would not apply here.
Comment 6 Maciej Stachowiak 2007-06-25 20:11:52 PDT
Comment on attachment 15051 [details]
cleaned up patch

Fix looks good to me. Please add a layout test. I believe this could be tested using dumpBackForwardList().

The comment is subtly wrong:

+            // We only add the main frames to the history


We do, in fact, add loads of frames other than the main frame to the history, if they are not initial loads, but that would not apply here.
Comment 7 Maciej Stachowiak 2007-06-25 20:11:55 PDT
Comment on attachment 15051 [details]
cleaned up patch

Fix looks good to me. Please add a layout test. I believe this could be tested using dumpBackForwardList().

The comment is subtly wrong:

+            // We only add the main frames to the history


We do, in fact, add loads of frames other than the main frame to the history, if they are not initial loads, but that would not apply here.
Comment 8 Maciej Stachowiak 2007-06-25 20:12:01 PDT
Comment on attachment 15051 [details]
cleaned up patch

Fix looks good to me. Please add a layout test. I believe this could be tested using dumpBackForwardList().

The comment is subtly wrong:

+            // We only add the main frames to the history


We do, in fact, add loads of frames other than the main frame to the history, if they are not initial loads, but that would not apply here.
Comment 9 Maxime BRITTO 2007-06-26 02:03:31 PDT
I was trying to create a test layout when I noticed I can't reproduce the bug anymore (even without the fix I proposed...).
I think my patch is no longer usefull :'(
Comment 10 Oliver Hunt 2007-07-14 16:32:43 PDT
This sounds like it is no longer reproducible, i certainly can't repro with the steps given.  

Am marking as worksforme, Geoffrey if you can still reproduce this reopen the bug, maybe reproduction requires an account?

Max are you sure that you were able to repro, and are no longer able to?
Comment 11 Geoffrey F. Green 2007-07-14 18:31:28 PDT
I am away from a Mac, but I'll check on Monday as soon as I return. I was still seeing the error on a nightly as recently as last week, but I'll be able to be more precise in a couple of days.
Comment 12 Maxime BRITTO 2007-07-16 01:35:18 PDT
(In reply to comment #10)
> Max are you sure that you were able to repro, and are no longer able to?
> 

Yes I'm sure I was able to repro it (I've worked on it and I've proposed a patch about it). But now I can't repro it anymore.
The only thing I changed since may be Safari (from 2.0.4 to 3.0) but I don't think it's related.
Comment 13 Geoffrey F. Green 2007-07-18 15:27:47 PDT
Original reporter here. Unable to reproduce with r24399.  It was intermittently reproducible as of a couple of nightlies ago, but before I had the chance to pin it down, it vanished completely. Looks like it's been fixed.  I'll keep checking and will update if it recurs.