1259574 - Make sure we're seeing disableglobalhistory on extra browsers that we don't want to record visits with

Status: RESOLVED WORKSFORME

(Firefox :: General, defect)

Description

[Mike Conley (:mconley) (:⚙️)]

disableglobalhistory is an attribute that we support with <xul:browser>'s that cause them to set useGlobalHistory to false inside their docShell's on construction. That means that when those browsers visit pages, it's not recorded in the Places database.

In bug 1254865, I fixed a bug where disableglobalhistory wasn't being supported properly by remote browsers that didn't have the remote-browser.xml binding attached.

This made me realize that there might be more browsers around the front-end that are having their visits recorded into Places that really shouldn't. Hello, Social API, Print Preview and View Source are just a few places that I know that use <xul:browser> that might not want visits recorded in Places.

So we should make sure all of those uses get disableglobalhistory set on them if appropriate.

Comment 1

[Mike Conley (:mconley) (:⚙️)]

Here are the suspicious browsers that I've found using DXR:


We have a few cases here:

1) Browsers don't have disablehistory or disableglobalhistory, and I think they should:
https://dxr.mozilla.org/mozilla-central/rev/6202ade0e6d688ffb67932398e56cfc6fa04ceb3/toolkit/components/viewsource/content/viewSource.js#711
https://dxr.mozilla.org/mozilla-central/rev/6202ade0e6d688ffb67932398e56cfc6fa04ceb3/toolkit/components/gfx/SanityTest.js#197
https://dxr.mozilla.org/mozilla-central/rev/6202ade0e6d688ffb67932398e56cfc6fa04ceb3/toolkit/components/viewsource/content/viewSource.xul#226

2) Browsers don't have disablehistory or disableglobalhistory, and I'm not sure if they should:
https://dxr.mozilla.org/mozilla-central/rev/6202ade0e6d688ffb67932398e56cfc6fa04ceb3/browser/modules/PanelFrame.jsm#61
https://dxr.mozilla.org/mozilla-central/rev/6202ade0e6d688ffb67932398e56cfc6fa04ceb3/browser/base/content/web-panels.xul#68
https://dxr.mozilla.org/mozilla-central/rev/6202ade0e6d688ffb67932398e56cfc6fa04ceb3/layout/tools/layout-debug/ui/content/layoutdebug.xul#180
https://dxr.mozilla.org/mozilla-central/rev/6202ade0e6d688ffb67932398e56cfc6fa04ceb3/layout/tools/recording/recording.xul#12
https://dxr.mozilla.org/mozilla-central/rev/6202ade0e6d688ffb67932398e56cfc6fa04ceb3/toolkit/components/help/content/help.xul#267

3) Browsers have disablehistory set on them, which is fine for non-remote browsers, but if someday we transition them to be remote browsers, they'll stop working, since it doesn't look like disablehistory is supported by remote browsers.
https://dxr.mozilla.org/mozilla-central/rev/6202ade0e6d688ffb67932398e56cfc6fa04ceb3/browser/base/content/browser-social.js#345
https://dxr.mozilla.org/mozilla-central/rev/6202ade0e6d688ffb67932398e56cfc6fa04ceb3/browser/base/content/browser.xul#1047
https://dxr.mozilla.org/mozilla-central/rev/6202ade0e6d688ffb67932398e56cfc6fa04ceb3/toolkit/components/viewsource/content/viewPartialSource.xul#158
https://dxr.mozilla.org/mozilla-central/rev/6202ade0e6d688ffb67932398e56cfc6fa04ceb3/browser/base/content/sync/setup.xul#203
https://dxr.mozilla.org/mozilla-central/rev/6202ade0e6d688ffb67932398e56cfc6fa04ceb3/browser/components/preferences/in-content/preferences.xul#200
https://dxr.mozilla.org/mozilla-central/rev/6202ade0e6d688ffb67932398e56cfc6fa04ceb3/toolkit/mozapps/extensions/content/extensions.xul#260

There is also the "view source browser" which is opened in a tab, and might need to be able to transition between using global history and not.

Comment 2

[Mike Conley (:mconley) (:⚙️)]

Filed bug 1259585 to get disablehistory support for remote browsers.

Comment 3

[Mike Conley (:mconley) (:⚙️)]

Hey Standard8, do you know if we're supposed to be recording history for users of the PanelFrame.jsm? The way it currently stands, I believe anything the PanelFrame browsers browse to will be added to the Places database.

Comment 4

[Mike Conley (:mconley) (:⚙️)]

Hey jryans, do you know if the web-panels.xul browser is supposed to be recording history to Places? This one right here: https://dxr.mozilla.org/mozilla-central/rev/6202ade0e6d688ffb67932398e56cfc6fa04ceb3/browser/base/content/web-panels.xul#68

Comment 5

[Mike Conley (:mconley) (:⚙️)]

Here's another one for Standard8:

https://dxr.mozilla.org/mozilla-central/source/browser/extensions/loop/chrome/content/modules/MozLoopService.jsm#1977

Comment 6

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #4)
> Hey jryans, do you know if the web-panels.xul browser is supposed to be
> recording history to Places? This one right here:
> https://dxr.mozilla.org/mozilla-central/rev/
> 6202ade0e6d688ffb67932398e56cfc6fa04ceb3/browser/base/content/web-panels.
> xul#68

I don't know anything about web-panels.xul, so I am not sure.  Is this the thing that lets you view a bookmark in a sidebar?  If so, maybe it should record history...?  But I am just making guesses.

Comment 7

[Mike Conley (:mconley) (:⚙️)]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #6)
> (In reply to Mike Conley (:mconley) - Needinfo me! from comment #4)
> > Hey jryans, do you know if the web-panels.xul browser is supposed to be
> > recording history to Places? This one right here:
> > https://dxr.mozilla.org/mozilla-central/rev/
> > 6202ade0e6d688ffb67932398e56cfc6fa04ceb3/browser/base/content/web-panels.
> > xul#68
> 
> I don't know anything about web-panels.xul, so I am not sure.  Is this the
> thing that lets you view a bookmark in a sidebar?  If so, maybe it should
> record history...?  But I am just making guesses.

Whoops - for some reason, I misread the path to that file and thought web-panels was part of devtools for some reason. Sorry for the noise.

Comment 8

[Mark Banner (:standard8)]

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #3)
> Hey Standard8, do you know if we're supposed to be recording history for
> users of the PanelFrame.jsm? The way it currently stands, I believe anything
> the PanelFrame browsers browse to will be added to the Places database.

We probably should set it there, but its probably not too much of an issue for Hello, maybe for social. For Hello the only thing we load in there is about:looppanel.

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #5)
> Here's another one for Standard8:
> 
> https://dxr.mozilla.org/mozilla-central/source/browser/extensions/loop/
> chrome/content/modules/MozLoopService.jsm#1977

Again we just load a chrome uri.

However, we should probably set it in both of these just to be sure.

Comment 9

[Mike Conley (:mconley) (:⚙️)]

Attachment: MozReview Request: Bug 1259574 - Disable docShell.useGlobalHistory on view source window browsers. r?jryans

Review commit: https://reviewboard.mozilla.org/r/42333/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/42333/

Comment 10

[Mike Conley (:mconley) (:⚙️)]

Attachment: MozReview Request: Bug 1259574 - Set disableglobalhistory on SanityTest browser. r?mchang

Review commit: https://reviewboard.mozilla.org/r/42335/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/42335/

Comment 11

[Mike Conley (:mconley) (:⚙️)]

Attachment: MozReview Request: Bug 1259574 - Set disableglobalhistory on Loop / PanelFrame browsers. r?Standard8

Review commit: https://reviewboard.mozilla.org/r/42337/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/42337/

Comment 12

[Mason Chang [Inactive] [:mchang]]

Comment on attachment 8734530 [details]
MozReview Request: Bug 1259574 - Set disableglobalhistory on SanityTest browser. r?mchang

https://reviewboard.mozilla.org/r/42335/#review38987

Comment 13

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8734529 [details]
MozReview Request: Bug 1259574 - Disable docShell.useGlobalHistory on view source window browsers. r?jryans

https://reviewboard.mozilla.org/r/42333/#review39001

::: toolkit/components/viewsource/content/viewSource-content.js:78
(Diff revision 1)
> +   * to be entered into the global history database, so we turn off
> +   * global history support once we show the source. We stash the
> +   * original value of whether or not we were using the global history
> +   * service here so that we can restore it when we pagehide.
> +   */
> +  oldUseGlobalHistory: undefined,

Do you think it's worth the trouble of maintaining this state?

It looks like we explicitly ignore view-source URLs in the history service, so maybe we should leave it alone?

https://dxr.mozilla.org/mozilla-central/rev/d5f3da0cfe7ccf846c354014c9b059fad6ba0de5/toolkit/components/places/nsNavHistory.cpp#1124

Comment 14

[Mike Conley (:mconley) (:⚙️)]

Ah, thanks jryans for pointing that out to me.

Considering that the browsers that Standard8 has been asked to review only load about: or chrome: pages, the changes I'm requesting from him are also probably not necessary.

Same with the SanityTest thing I got mchang to review.

I think this is actually RESOLVED WORKSFORME - I can't find any other browsers that we might want to care about setting disableglobalhistory on here.