931421 - Enable useGlobalHistory for child process docshells

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: use-global-history

I feel silly not doing this earlier. This appears to be all we need in order to make the places database work in e10s, along with all the related awesome bar functionality.

Comment 1

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 822834 [details] [diff] [review]
use-global-history

As content.js always runs (even for non-e10s), I think we can't do this here, because this would enable globalHistory for every browser, but there are cases where that can be disabled: http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/browser.xml#699

So I think the best place to do this would be in browser-child.js which only runs for remote browsers.  (and maybe it's a good idea to try to respect disablehistory too?)

I'm curious, do you know how that flag is enough to make these things work? I'm curious into how setting that flag in the child can make the awesomebar work in the parent.. (probably because I'm not sure what you mean by "related awesomebar features")

Comment 2

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: use-global-history v2

I moved it to browser-child.js. I don't think there's any need to check for the disablehistory attribute. We'll never set that attribute on a <browser> element where browser-child.js is loaded.

The useGlobalHistory flag is used to determine whether the docshell should put URLs in the places database:
  http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#11496

If we do put the URL in the places database, that ends up calling some indexeddb code in the child to do the insertion. That request gets forwarded to the parent, which actually stores it in the database. The awesomebar only displays things that are in the places database.

Comment 3

[:Gavin Sharp [email: [e-mail]]]

(In reply to Bill McCloskey (:billm) from comment #2)
> We'll never set that attribute on a <browser>
> element where browser-child.js is loaded.

I don't think this assumption will hold in the long term. If it's easy to fix, might as well; if it's a pain, file a followup?

Comment 4

[:Gavin Sharp [email: [e-mail]]]

The comment could be better, too. Not "for the places db to work", but rather "for docshell to save things in the places DB". I would probably omit the comment though, seems fairly self-explanatory.

Comment 5

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Could you explain these attributes, Gavin? I only just now realized that there are two separate ones.

The disablehistory attribute seems like it's maybe just an optimization? It avoids creating the nsSHistory object. Does that cause any differences in behavior? The cost of having an nsSHistory object seems fairly small. It's cheap to create, and the browser elements that we set the attribute on are used infrequently, so the ongoing cost of keeping the history up to date seems small.

I can see now how disableglobalhistory could be useful though. Is the idea that <browser> elements for sidebars shouldn't put entries in the places database? If so, I guess we can find a way to propagate the attribute to the child.

Comment 6

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: use-global-history v3

This one respects the disableglobalhistory attribute.

Comment 7

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 823113 [details] [diff] [review]
use-global-history v3

(In reply to Bill McCloskey (:billm) from comment #5)
> Could you explain these attributes, Gavin? I only just now realized that
> there are two separate ones.
> 
> The disablehistory attribute seems like it's maybe just an optimization? It
> avoids creating the nsSHistory object. Does that cause any differences in
> behavior? The cost of having an nsSHistory object seems fairly small. It's
> cheap to create, and the browser elements that we set the attribute on are
> used infrequently, so the ongoing cost of keeping the history up to date
> seems small.

I dug up the history on this.. It was not exactly added as an optimization, but as a hack workaround for not being able to setup session history for non-root docshells.  It does not affect <browser type="content"> because it's a root docshell, but it caused problems with a privileged sidebar like the bookmarks or history sidebar panes.
(From bug 412171 comment 17, and bug 408668)
With that, I think it's unecessary to respect disablehistory here. If Gavin think that is important we can add as a follow up. Respecting disableglobalhistory is necessary though, as is done by this patch.

> 
> I can see now how disableglobalhistory could be useful though. Is the idea
> that <browser> elements for sidebars shouldn't put entries in the places
> database? If so, I guess we can find a way to propagate the attribute to the
> child.

Yep, exactly.

Comment 8

[:Felipe Gomes (needinfo for replies!)]

http://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser.css#l21 was added to avoid other users of remote browsers to use remote-browser.xml, so I think they are safe in not needing to set the attribute.  But adding markh just to verify.

Mark, with this, remote browsers (through remote-browser.xml) will start to add their entries to the places history, unless otherwise marked with disableglobalhistory.  Do you think the browsers used for the thumbnail service/social framework need to be updated for that?

Comment 9

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Thanks for digging up those bugs, Felipe.
https://hg.mozilla.org/integration/mozilla-inbound/rev/9583bd3099ae

Comment 10

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

(In reply to :Felipe Gomes from comment #8)
> Mark, with this, remote browsers (through remote-browser.xml) will start to
> add their entries to the places history, unless otherwise marked with
> disableglobalhistory.  Do you think the browsers used for the thumbnail
> service/social framework need to be updated for that?

No, we don't use the binding so should be good.  I don't think thumbnails have explicit tests for history, but social does and it is moving towards remote browsers (also without the binding), so they should pick up potential future refactorings that might cause future problems...

CCing Drew anyway, just incase he thinks it worthwhile to add thumbnail tests that ensure we don't munge history

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9583bd3099ae