Closed Bug 1588479 Opened 5 years ago Closed 4 years ago

[Fission] iframe URLs show up in History, which causes them to show as Top Sites on about:newtab

Categories

(Core :: DOM: Navigation, defect, P2)

Desktop
All
defect

Tracking

()

RESOLVED DUPLICATE of bug 1594529
Fission Milestone M5b
Tracking Status
firefox71 --- affected

People

(Reporter: Gabi, Assigned: annyG)

References

Details

Attachments

(1 file)

Attached image historyandtopsite.png
  • [Affected versions]:
    Fx 71.0a1

  • [Affected platforms]:
    macOS 10.14
    Windows 10x64
    Ubuntu 16.4 x64

  • [Steps to reproduce]:

  1. Launch Firefox
  2. Navigate to about:config and set pref fission.autostart to true
  3. Restart the browser (CTRL+ALT+R in the browser console)
  4. Go to any web page (reddit.com)
  5. Restart the browser again
  6. Show History from address bar
  7. Observe the history

[Expected result]:
Reddit.com should be displayed in the history

[Actual result]:
Lots of cookies are present in history, after a while these cookies are set as top sites.

Is this known because of the work with history?

Flags: needinfo?(nkochar)

This is probably happening because each iframe is loading in its process and is somehow identified as top load. Discussed this with Peter, and it could be history or session store. We'll look into this when we fix the dogfooding bugs.

Flags: needinfo?(nkochar)
Fission Milestone: --- → M5
Priority: -- → P2
Summary: [Fission] History is populated with site cookies and after a while these cookies are set as top sites → [Fission] iframe URLs show up in History, which causes them to show as Top Sites on about:newtab

I'm going to give this a try. The place where we add items to recently visited tabs and windows is History::VisitURI, which gets called inside of nsDocShell::AddURIVisit. Earlier in the method, we set the top level load flag depending on whether the docshell is a frame or not.

I speculated that we don't set this accurately or at a correct time, and I found that we only set it once, when we first create the docshell in nsFrameLoader::MaybeCreateDocshell. I figured that I would test whether some part of my theory is correct by changing the condition under which we set the top level load flag from if (!IsFrame()) to if (!mBrowsingContext->GetParent() && !IsFrame()), essentially accounting for the fact that maybe since the time of the creation of the docshell, we now have a parent and forgot to set it. With this change I am no longer seeing iframes in the list of last visited tabs/windows but I am seeing two entries for websites when I go to non-http version and I suspect it is because of redirects from http version of the website to https.

This is not the right fix because we should be setting IsFrame() correctly from the beginning, so I will investigate why that is not the case.

Upon debugging I discovered that the docshell for one of the iframes that showed up in the list was created in nsWebBrowser::Create. I see that neither in nsDocShell::Create nor later in nsWebBrowser::Create we set mIsFrame so I wonder if that is the problem? Inside of nsDocShell::Create I added this

  if (parent) {
    ds->SetIsFrame();
  }

and initial testing shows that iframes no longer show up in the list of recently visited tabs/windows. However, I still get a double entries in the list when the http version of the site is redirected to https, so I will continue investigating.

Some of my next steps are:

I spoke to smaug this morning and he pointed out a few things

  • Perhaps we should change IsFrame() to check if the browsing context has a parent, instead of storing that value in a field.
  • If my speculations are correct that we are not setting mIsFrame correctly, how come we didn't have any failing tests? This would mean we need a lot of tests iframes. I'll pay attention to test failure when I post to try.
  • Perhaps http and https URLs showing up in the list of recently visited is a different issue (e.g. maybe front-end code). I need to figure out how closely related the two problems are.

Currently I'm waiting for treeherder tests to finish running to see how much damage is caused by changing nsDocShell::IsFrame(). Also, I tested that without fission http->https redirects show up as double entry as well, so perhaps this is a non Fission issue after all.
I am also noticing that mBrowsingContext->GetParent() sometimes gets used in docshell to see if we are a subframe, so I need to remember to replace such calls with a call to IsFrame, which I think would actually be better named as IsSubFrame().

Assignee: nobody → agakhokidze
Status: NEW → ASSIGNED
Depends on: 1594529

Anny confirmed that this bug should still block Fission dogfooding (M5). It will be fixed soon by TYPE_DOCUMENT bug 1594529.

Moving P2 M5 bugs to M5b milestone

Fission Milestone: M5 → M5b

Anny fixed this bug in TYPE_DOCUMENT bug 1594529.

Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → DUPLICATE
You need to log in before you can comment on or make changes to this bug.