Bug 13686 – REGRESSION: Certain iframes embedded in page listed in History menu

• Component: History
• Status: RESOLVED WORKSFORME

[Geoffrey F. Green]

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)

[Geoffrey F. Green]

Created attachment 14499 [details]
Screenshot of History menu

[David Kilzer (:ddkilzer)]

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.

[Maxime BRITTO]

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.

[Maxime BRITTO]

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.

[Maciej Stachowiak]

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.

[Maciej Stachowiak]

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.

[Maciej Stachowiak]

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.

[Maciej Stachowiak]

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.

[Maxime BRITTO]

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 :'(

[Oliver Hunt]

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?

[Geoffrey F. Green]

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.

[Maxime BRITTO]

(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.

[Geoffrey F. Green]

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.