Open Bug 1209662 Opened 4 years ago Updated 8 months ago

Support session history navigation between different processes for new security model process isolation

Categories

(Core :: DOM: Core & HTML, defect, P2)

defect

Tracking

()

ASSIGNED
FxOS-S7 (18Sep)
Tracking Status
firefox44 --- affected

People

(Reporter: kanru, Assigned: kanru)

References

Details

Attachments

(13 files)

40 bytes, text/x-review-board-request
ttaubert
: review+
Details
40 bytes, text/x-review-board-request
ttaubert
: review+
Details
40 bytes, text/x-review-board-request
ttaubert
: review+
Details
40 bytes, text/x-review-board-request
fabrice
: review+
Details
40 bytes, text/x-review-board-request
fabrice
: review+
ttaubert
: review+
Details
40 bytes, text/x-review-board-request
fabrice
: review+
ttaubert
: review+
Details
40 bytes, text/x-review-board-request
fabrice
: review+
ttaubert
: review+
Details
40 bytes, text/x-review-board-request
smaug
: review+
Details
40 bytes, text/x-review-board-request
ttaubert
: review+
Details
40 bytes, text/x-review-board-request
smaug
: review-
Details
40 bytes, text/x-review-board-request
smaug
: review+
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
smaug
: review-
Details
+++ This bug was initially created as a clone of Bug #1033999 +++

Bug 1033999 is for generic session store support so I create this new bug for the parts needed by new security model to implement collecting and restoring session history when we switches processes.

The plan is to reuse the SessionHistory.jsm from desktop and add a new TabId field to SHEntry and use the field for moving forward/backward in the history that corresponding to different processes.
Blocks: 1219637
Bug 1209662 - Move browser/components/sessionstore/Utils.jsm to toolkit/modules/sessionstore.
Bug 1209662 - Move browser/components/sessionstore/SessionHistory.jsm to toolkit/modules/sessionstore.
Bug 1209662 - Move browser/components/sessionstore/FrameTree.jsm to toolkit/modules/sessionstore.
Attachment #8690728 - Flags: review?(ttaubert)
Comment on attachment 8690728 [details]
MozReview Request: Bug 1209662 - Move browser/components/sessionstore/Utils.jsm to toolkit/modules/sessionstore. r?ttaubert

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25915/diff/1-2/
Comment on attachment 8690729 [details]
MozReview Request: Bug 1209662 - Move browser/components/sessionstore/SessionHistory.jsm to toolkit/modules/sessionstore. r?ttaubert

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25917/diff/1-2/
Attachment #8690729 - Flags: review?(ttaubert)
Comment on attachment 8690730 [details]
MozReview Request: Bug 1209662 - Move browser/components/sessionstore/FrameTree.jsm to toolkit/modules/sessionstore. r?ttaubert

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25919/diff/1-2/
Attachment #8690730 - Flags: review?(ttaubert)
Comment on attachment 8690732 [details]
MozReview Request: Bug 1209662 - Collect session history data for browser element, child side r?ttaubert,fabrice

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25921/diff/1-2/
Attachment #8690732 - Flags: review?(ttaubert)
Attachment #8690732 - Flags: review?(fabrice)
Attachment #8690733 - Flags: review?(ttaubert)
Attachment #8690733 - Flags: review?(fabrice)
Comment on attachment 8690733 [details]
MozReview Request: Bug 1209662 - Collect session history data for browser element, parent side r?ttaubert,fabrice

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25923/diff/1-2/
Attachment #8690734 - Flags: review?(ttaubert)
Attachment #8690734 - Flags: review?(fabrice)
Comment on attachment 8690734 [details]
MozReview Request: Bug 1209662 - Restore session history data after switch process, parent side r?ttaubert,fabrice

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25925/diff/1-2/
Attachment #8690735 - Flags: review?(ttaubert)
Attachment #8690735 - Flags: review?(fabrice)
Comment on attachment 8690735 [details]
MozReview Request: Bug 1209662 - Restore session history data after switch process, child side r?ttaubert,fabrice

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25927/diff/1-2/
Attachment #8690736 - Flags: review?(bugs)
Comment on attachment 8690736 [details]
MozReview Request: Bug 1209662 - Add a TabId attribute to SHEntry. r?smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25929/diff/1-2/
Attachment #8690738 - Flags: review?(ttaubert)
Comment on attachment 8690738 [details]
MozReview Request: Bug 1209662 - Restore TabId of SHEntry in SessionHistory.jsm r?ttaubert

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25931/diff/1-2/
Comment on attachment 8690739 [details]
MozReview Request: Bug 1209662 - Implement history navigation between processes r?smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25933/diff/1-2/
Attachment #8690739 - Flags: review?(bugs)
Hi Tim, Fabrice and Olli: this is a WIP patch series implementing history navigation between processes so please consider this a f? instead of r?

The idea is to store a process id in SEntry so every processes sharing the same history could look up the id and switch to the corresponding process or load from current process. It's very similiar to the BFCache handling.

Tim and Fabrice, what do you think about the session restore part? Olli, do you like the process switch approach?
Comment on attachment 8690736 [details]
MozReview Request: Bug 1209662 - Add a TabId attribute to SHEntry. r?smaug

(just testing if I can move this to feedback queue)
Attachment #8690736 - Flags: review?(bugs) → feedback?(bugs)
Attachment #8690739 - Flags: review?(bugs) → feedback?(bugs)
Attachment #8690728 - Flags: review?(ttaubert)
Comment on attachment 8690728 [details]
MozReview Request: Bug 1209662 - Move browser/components/sessionstore/Utils.jsm to toolkit/modules/sessionstore. r?ttaubert

https://reviewboard.mozilla.org/r/25915/#review23337

This is fine in general but why doesn't ReviewBoard show the files as simply moved? I don't want to manually check whether the files are still the same and we shouldn't lose blame information.
Attachment #8690729 - Flags: review?(ttaubert)
Comment on attachment 8690729 [details]
MozReview Request: Bug 1209662 - Move browser/components/sessionstore/SessionHistory.jsm to toolkit/modules/sessionstore. r?ttaubert

https://reviewboard.mozilla.org/r/25917/#review23339

Should move the file properly here too.
Attachment #8690730 - Flags: review?(ttaubert)
Comment on attachment 8690730 [details]
MozReview Request: Bug 1209662 - Move browser/components/sessionstore/FrameTree.jsm to toolkit/modules/sessionstore. r?ttaubert

https://reviewboard.mozilla.org/r/25919/#review23341

Same comment about moving the file.
Comment on attachment 8690736 [details]
MozReview Request: Bug 1209662 - Add a TabId attribute to SHEntry. r?smaug

Looks like a simple change to SHEntry.
I guess the other patches show how it is actually going to be used.

But the attribute doesn't need to readonly and then you can remove
void setTabId(in unsigned long aId);

And the attribute needs to be unsigned long long, not unsigned long.
and uint64_t mTabId; in nsSHEntry
Attachment #8690736 - Flags: feedback?(bugs) → review+
Comment on attachment 8690739 [details]
MozReview Request: Bug 1209662 - Implement history navigation between processes r?smaug

+  if (mSessionHistory && aSHEntry && aLoadType == LOAD_HISTORY) {
could you perhaps move this few lines down to the 'if'
which has aLoadType == LOAD_HISTORY check.

+      nsCOMPtr<nsIURI> uri;
+      rv = NS_NewURI(getter_AddRefs(uri), "about:blank");
+      if (NS_FAILED(rv)) {
+        return NS_ERROR_FAILURE;
+      }
+      tab->SendSwitchProcessAndGotoIndex(tabId, idx);
+      return NS_OK;
So we don't actually load that about:blank.
And you should null check 'tab'.

mSessionHistory is set only in top level content docshell. Have you tested how this all works in
case there are iframes? Though, I guess we agreed that this all won't deal with iframes anyway, only
top level history loads.

it is not clear to me how index in session history is handled cross processes.
There are fun cases like page loads:
page-in-process1, page2-in-process1, page-in-process2, page2-in-process2, page-in-process3, page2-in-process3, and then history.go(-5).
How does process1 know to load page2?
Attachment #8690739 - Flags: feedback?(bugs)
Comment on attachment 8690732 [details]
MozReview Request: Bug 1209662 - Collect session history data for browser element, child side r?ttaubert,fabrice

https://reviewboard.mozilla.org/r/25921/#review24047

::: dom/browser-element/BrowserElementChildSessionStore.js:93
(Diff revision 1)
> +  send: function (options = {}) {

Maybe I missed something but it looks like we never call send() with options? Or do you plan something for later?

::: dom/browser-element/BrowserElementChildSessionStore.js:150
(Diff revision 1)
> +    docShell.QueryInterface(Ci.nsIWebNavigation).sessionHistory.

Here you don't check that sessionHistory is not null while you do in uninit()
Attachment #8690732 - Flags: review?(fabrice)
Comment on attachment 8690733 [details]
MozReview Request: Bug 1209662 - Collect session history data for browser element, parent side r?ttaubert,fabrice

https://reviewboard.mozilla.org/r/25923/#review24053
Attachment #8690733 - Flags: review?(fabrice) → review+
Comment on attachment 8690732 [details]
MozReview Request: Bug 1209662 - Collect session history data for browser element, child side r?ttaubert,fabrice

https://reviewboard.mozilla.org/r/25921/#review24055
Attachment #8690732 - Flags: review+
Comment on attachment 8690734 [details]
MozReview Request: Bug 1209662 - Restore session history data after switch process, parent side r?ttaubert,fabrice

https://reviewboard.mozilla.org/r/25925/#review24057
Attachment #8690734 - Flags: review?(fabrice) → review+
Attachment #8690735 - Flags: review?(fabrice) → review+
Comment on attachment 8690735 [details]
MozReview Request: Bug 1209662 - Restore session history data after switch process, child side r?ttaubert,fabrice

https://reviewboard.mozilla.org/r/25927/#review24059

::: dom/browser-element/BrowserElementChildSessionStore.js:254
(Diff revision 1)
> +  // to ensure that docshell doeesn't get confused. Don't bother doing this if

nit: s/doeesn't/doesn't
Comment on attachment 8690732 [details]
MozReview Request: Bug 1209662 - Collect session history data for browser element, child side r?ttaubert,fabrice

https://reviewboard.mozilla.org/r/25921/#review26253

::: dom/browser-element/BrowserElementChildSessionStore.js:112
(Diff revision 1)
> +    let sendMessage = sync ? sendRpcMessage : sendAsyncMessage;

Fabrice is right, we don't need all that stuff here, we won't send sync messages, right?

Also, the MQ is quite generic to support all kinds of data that SessionStore wants to collect but that seems not needed here?

::: dom/browser-element/BrowserElementChildSessionStore.js:114
(Diff revision 1)
> +    let data = {};
> +    for (let [key, lazyfn] of this._data) {
> +      data[key] = lazyfn();
> +    }

There's only one kind of data.

::: dom/browser-element/BrowserElementChildSessionStore.js:120
(Diff revision 1)
> +    sendMessage("SessionStore:update", {
> +      data, isFinal: options.isFinal || false
> +    });

You don't care about / support the isFinal flag, right?
Attachment #8690732 - Flags: review?(ttaubert)
Comment on attachment 8690733 [details]
MozReview Request: Bug 1209662 - Collect session history data for browser element, parent side r?ttaubert,fabrice

https://reviewboard.mozilla.org/r/25923/#review26257

::: dom/browser-element/BrowserElementParent.js:358
(Diff revision 1)
> +    case 'SessionStore:update':
> +      this._handleSessionStoreUpdate(aMsg);
> +      break;
> +    }

Should the message name be more specific? session-history:update or something?

::: dom/browser-element/BrowserElementParent.js:364
(Diff revision 1)
> +  _handleSessionStoreUpdate: function(aMsg) {
> +    this._sessionStore = aMsg.data;
> +  },

Same for function names. I'd also rather name it _sessionHistory.
Attachment #8690733 - Flags: review?(ttaubert)
Comment on attachment 8690734 [details]
MozReview Request: Bug 1209662 - Restore session history data after switch process, parent side r?ttaubert,fabrice

https://reviewboard.mozilla.org/r/25925/#review26261

::: dom/browser-element/BrowserElementParent.js:309
(Diff revision 1)
> +  restoreSessionData: function() {
> +    this._mm.sendAsyncMessage("SessionStore:restore", this._sessionStore);
> +  },

session-history:restore? And all other function names should probably also reflect that it's only about session history.
Attachment #8690734 - Flags: review?(ttaubert)
Comment on attachment 8690735 [details]
MozReview Request: Bug 1209662 - Restore session history data after switch process, child side r?ttaubert,fabrice

https://reviewboard.mozilla.org/r/25927/#review26263

::: dom/browser-element/BrowserElementChildSessionStore.js:242
(Diff revision 1)
> +addMessageListener("SessionStore:restore", aMsg => {

Same comment about message name.

::: dom/browser-element/BrowserElementChildSessionStore.js:252
(Diff revision 1)
> +  // Make sure currentURI is set so that switch-to-tab works before the tab is
> +  // restored. We'll reset this to about:blank when we try to restore the tab
> +  // to ensure that docshell doeesn't get confused. Don't bother doing this if
> +  // we're restoring immediately due to a process switch. It just causes the
> +  // URL bar to be temporarily blank.

This comment pretty much tells you why we don't need the .setCurrentURI() call below :)
Attachment #8690735 - Flags: review?(ttaubert)
Comment on attachment 8690738 [details]
MozReview Request: Bug 1209662 - Restore TabId of SHEntry in SessionHistory.jsm r?ttaubert

https://reviewboard.mozilla.org/r/25931/#review26267
Attachment #8690738 - Flags: review?(ttaubert) → review+
(In reply to Tim Taubert [:ttaubert] from comment #34)
> Comment on attachment 8690732 [details]
> MozReview Request: Bug 1209662 - Collect session history data for browser
> element, child side
> 
> https://reviewboard.mozilla.org/r/25921/#review26253
> 
> ::: dom/browser-element/BrowserElementChildSessionStore.js:112
> (Diff revision 1)
> > +    let sendMessage = sync ? sendRpcMessage : sendAsyncMessage;
> 
> Fabrice is right, we don't need all that stuff here, we won't send sync
> messages, right?
> 
> Also, the MQ is quite generic to support all kinds of data that SessionStore
> wants to collect but that seems not needed here?

This and other stuff that doesn't seem to relate to session history restore were added because the intention to support restore other kind of data in the future. I can remove them to simplify the code and add them back when needed.
(In reply to Kan-Ru Chen [:kanru] (UTC+8) from comment #39)
> (In reply to Tim Taubert [:ttaubert] from comment #34)
> > Also, the MQ is quite generic to support all kinds of data that SessionStore
> > wants to collect but that seems not needed here?
> 
> This and other stuff that doesn't seem to relate to session history restore
> were added because the intention to support restore other kind of data in
> the future. I can remove them to simplify the code and add them back when
> needed.

Hmm, I see. Guess it makes sense to leave it in then? OTOH, when the times comes, there would be an incentive to share code between fxos and desktop, instead of using the duplicated code.
(In reply to Tim Taubert [:ttaubert] from comment #40)
> (In reply to Kan-Ru Chen [:kanru] (UTC+8) from comment #39)
> > (In reply to Tim Taubert [:ttaubert] from comment #34)
> > > Also, the MQ is quite generic to support all kinds of data that SessionStore
> > > wants to collect but that seems not needed here?
> > 
> > This and other stuff that doesn't seem to relate to session history restore
> > were added because the intention to support restore other kind of data in
> > the future. I can remove them to simplify the code and add them back when
> > needed.
> 
> Hmm, I see. Guess it makes sense to leave it in then? OTOH, when the times
> comes, there would be an incentive to share code between fxos and desktop,
> instead of using the duplicated code.

Yes, we will want to share more code. Actually for my use case I don't need a queue and want to send the history back to parent as soon as possible. I will remove the queue first.
Comment on attachment 8690728 [details]
MozReview Request: Bug 1209662 - Move browser/components/sessionstore/Utils.jsm to toolkit/modules/sessionstore. r?ttaubert

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25915/diff/1-2/
Attachment #8690728 - Attachment description: MozReview Request: Bug 1209662 - Move browser/components/sessionstore/Utils.jsm to toolkit/modules/sessionstore. → MozReview Request: Bug 1209662 - Move browser/components/sessionstore/Utils.jsm to toolkit/modules/sessionstore. r?ttaubert
Attachment #8690728 - Flags: review?(ttaubert)
Attachment #8690729 - Attachment description: MozReview Request: Bug 1209662 - Move browser/components/sessionstore/SessionHistory.jsm to toolkit/modules/sessionstore. → MozReview Request: Bug 1209662 - Move browser/components/sessionstore/SessionHistory.jsm to toolkit/modules/sessionstore. r?ttaubert
Attachment #8690729 - Flags: review?(ttaubert)
Comment on attachment 8690729 [details]
MozReview Request: Bug 1209662 - Move browser/components/sessionstore/SessionHistory.jsm to toolkit/modules/sessionstore. r?ttaubert

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25917/diff/1-2/
Comment on attachment 8690730 [details]
MozReview Request: Bug 1209662 - Move browser/components/sessionstore/FrameTree.jsm to toolkit/modules/sessionstore. r?ttaubert

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25919/diff/1-2/
Attachment #8690730 - Attachment description: MozReview Request: Bug 1209662 - Move browser/components/sessionstore/FrameTree.jsm to toolkit/modules/sessionstore. → MozReview Request: Bug 1209662 - Move browser/components/sessionstore/FrameTree.jsm to toolkit/modules/sessionstore. r?ttaubert
Attachment #8690730 - Flags: review?(ttaubert)
Attachment #8690732 - Attachment description: MozReview Request: Bug 1209662 - Collect session history data for browser element, child side → MozReview Request: Bug 1209662 - Collect session history data for browser element, child side r?ttaubert,fabrice
Attachment #8690732 - Flags: review?(ttaubert)
Comment on attachment 8690732 [details]
MozReview Request: Bug 1209662 - Collect session history data for browser element, child side r?ttaubert,fabrice

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25921/diff/1-2/
Comment on attachment 8690733 [details]
MozReview Request: Bug 1209662 - Collect session history data for browser element, parent side r?ttaubert,fabrice

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25923/diff/1-2/
Attachment #8690733 - Attachment description: MozReview Request: Bug 1209662 - Collect session history data for browser element, parent side → MozReview Request: Bug 1209662 - Collect session history data for browser element, parent side r?ttaubert,fabrice
Attachment #8690733 - Flags: review?(ttaubert)
Comment on attachment 8690734 [details]
MozReview Request: Bug 1209662 - Restore session history data after switch process, parent side r?ttaubert,fabrice

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25925/diff/1-2/
Attachment #8690734 - Attachment description: MozReview Request: Bug 1209662 - Restore session history data after switch process, parent side → MozReview Request: Bug 1209662 - Restore session history data after switch process, parent side r?ttaubert,fabrice
Attachment #8690734 - Flags: review?(ttaubert)
Comment on attachment 8690735 [details]
MozReview Request: Bug 1209662 - Restore session history data after switch process, child side r?ttaubert,fabrice

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25927/diff/1-2/
Attachment #8690735 - Attachment description: MozReview Request: Bug 1209662 - Restore session history data after switch process, child side → MozReview Request: Bug 1209662 - Restore session history data after switch process, child side r?ttaubert,fabrice
Attachment #8690735 - Flags: review?(ttaubert)
Comment on attachment 8690736 [details]
MozReview Request: Bug 1209662 - Add a TabId attribute to SHEntry. r?smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25929/diff/1-2/
Attachment #8690736 - Attachment description: MozReview Request: Bug 1209662 - Add a TabId attribute to SHEntry. → MozReview Request: Bug 1209662 - Add a TabId attribute to SHEntry. r?smaug
Attachment #8690736 - Flags: review+ → review?(bugs)
Comment on attachment 8690738 [details]
MozReview Request: Bug 1209662 - Restore TabId of SHEntry in SessionHistory.jsm r?ttaubert

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25931/diff/1-2/
Attachment #8690738 - Attachment description: MozReview Request: Bug 1209662 - Restore TabId of SHEntry in SessionHistory.jsm → MozReview Request: Bug 1209662 - Restore TabId of SHEntry in SessionHistory.jsm r?ttaubert
Comment on attachment 8690739 [details]
MozReview Request: Bug 1209662 - Implement history navigation between processes r?smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25933/diff/1-2/
Attachment #8690739 - Attachment description: MozReview Request: Bug 1209662 - Implement history navigation between processes → MozReview Request: Bug 1209662 - Implement history navigation between processes r?smaug
Attachment #8690739 - Flags: review?(bugs)
Attachment #8690740 - Attachment description: MozReview Request: Bug 1209662 - Add mochitest → MozReview Request: Bug 1209662 - Create a new process for navigation when the old one is missing. r?smaug
Attachment #8690740 - Flags: review?(bugs)
Comment on attachment 8690740 [details]
MozReview Request: Bug 1209662 - Create a new process for navigation when the old one is missing. r?smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25935/diff/1-2/
Attachment #8690732 - Flags: review+ → review?(fabrice)
Attachment #8690733 - Flags: review+ → review?(fabrice)
Attachment #8690734 - Flags: review+ → review?(fabrice)
Attachment #8690735 - Flags: review+ → review?(fabrice)
Comment on attachment 8709904 [details]
MozReview Request: Bug 1209662 - Support multiple entries in network.http.signed-packages.trusted-origin pref r?valentin

https://reviewboard.mozilla.org/r/31597/#review28319

::: netwerk/protocol/http/PackagedAppVerifier.cpp:99
(Diff revision 1)
> +      if (end == (uint32_t)-1) {

I think it's better to use kNotFound instead of -1.
Attachment #8709904 - Flags: review?(valentin.gosu)
Comment on attachment 8690728 [details]
MozReview Request: Bug 1209662 - Move browser/components/sessionstore/Utils.jsm to toolkit/modules/sessionstore. r?ttaubert

https://reviewboard.mozilla.org/r/25915/#review28467

A proper rename, yay!
Attachment #8690728 - Flags: review?(ttaubert) → review+
Attachment #8690729 - Flags: review?(ttaubert) → review+
Comment on attachment 8690729 [details]
MozReview Request: Bug 1209662 - Move browser/components/sessionstore/SessionHistory.jsm to toolkit/modules/sessionstore. r?ttaubert

https://reviewboard.mozilla.org/r/25917/#review28469

\o/
Comment on attachment 8690730 [details]
MozReview Request: Bug 1209662 - Move browser/components/sessionstore/FrameTree.jsm to toolkit/modules/sessionstore. r?ttaubert

https://reviewboard.mozilla.org/r/25919/#review28471
Attachment #8690730 - Flags: review?(ttaubert) → review+
Comment on attachment 8690733 [details]
MozReview Request: Bug 1209662 - Collect session history data for browser element, parent side r?ttaubert,fabrice

https://reviewboard.mozilla.org/r/25923/#review28473
Attachment #8690733 - Flags: review?(ttaubert) → review+
Attachment #8690734 - Flags: review?(ttaubert) → review+
Comment on attachment 8690734 [details]
MozReview Request: Bug 1209662 - Restore session history data after switch process, parent side r?ttaubert,fabrice

https://reviewboard.mozilla.org/r/25925/#review28475
Comment on attachment 8690735 [details]
MozReview Request: Bug 1209662 - Restore session history data after switch process, child side r?ttaubert,fabrice

https://reviewboard.mozilla.org/r/25927/#review28477
Attachment #8690735 - Flags: review?(ttaubert) → review+
Comment on attachment 8690732 [details]
MozReview Request: Bug 1209662 - Collect session history data for browser element, child side r?ttaubert,fabrice

https://reviewboard.mozilla.org/r/25921/#review28479

::: dom/browser-element/BrowserElementChildSessionStore.js:45
(Diff revisions 1 - 2)
> -    docShell.QueryInterface(Ci.nsIWebNavigation).sessionHistory.
> -      addSHistoryListener(this);
> +    let sessionHistory =
> +        docShell.QueryInterface(Ci.nsIWebNavigation).sessionHistory;
> +    if (sessionHistory) {
> +      sessionHistory.addSHistoryListener(this);
> +    }

When exactly is sessionHistory=null?

1) This doesn't really work with the SessionHistory.isEmpty() call below because that just expects it to be there.

2) What's the whole point of the SessionHistoryListener if it doesn't listen for shistory changes?

::: dom/browser-element/BrowserElementChildSessionStore.js:70
(Diff revisions 1 - 2)
> -      AsyncMessageQueue.push("history", () => SessionHistory.collect(docShell));
> +      sendAsyncMessage("session-history:update", {
> +        "history": SessionHistory.collect(docShell)
> +      });

I don't think this is what you want. shistory collection can take quite a while and you wouldn't want to do this whenever the page title changes or shistory changes.

Using the MessageQueue is probably the right thing, but you can reduce it to only the stuff you need.
Attachment #8690732 - Flags: review?(ttaubert)
Comment on attachment 8690736 [details]
MozReview Request: Bug 1209662 - Add a TabId attribute to SHEntry. r?smaug

https://reviewboard.mozilla.org/r/25929/#review29435

::: docshell/shistory/nsISHEntry.idl:319
(Diff revision 2)
> +    void setTabId(in unsigned long aId);

Either make setTabId non-scriptable, so that only docshell/* code ends up using it, or just
remove setTabId and drop readonly from tabId attribute.
Either way.
(I wonder if session store needs to set the attribute, in which case the latter approach would be needed.)

Also, looks like the id is stored elsewhere as  uint64_t, so use unsigned long long in idl and  uint64_t in c++ code.
Attachment #8690736 - Flags: review?(bugs) → review+
Attachment #8690740 - Flags: review?(bugs) → review+
Comment on attachment 8690740 [details]
MozReview Request: Bug 1209662 - Create a new process for navigation when the old one is missing. r?smaug

https://reviewboard.mozilla.org/r/25935/#review29439

So could you ask someone else to review that particular method. Other parts are ok.

::: dom/browser-element/BrowserElementParent.js:308
(Diff revision 2)
> +  tabIdUpdated: function(aOldId, aNewId) {

Does this update also the child entries of entry? Or what kind of entries these are? They aren't nsISHEntry objects, right?
I think I can't quite review this particular method since I don't know what this._sessionHistory is.
Attachment #8690739 - Flags: review?(bugs)
Comment on attachment 8690739 [details]
MozReview Request: Bug 1209662 - Implement history navigation between processes r?smaug

https://reviewboard.mozilla.org/r/25933/#review29459

So I think the docshell part need to be sorted out.

::: docshell/base/nsDocShell.cpp:9948
(Diff revision 2)
> +    if (tabId && tabId != tab->GetTabId()) {

Move this tabI != tab->GetTabId() check as the first thing in the 'if', so that we don't unnecessarily access mSessionHistory->GetRequestedIndex and such when we aren't going to use idx for anything anyway.

::: docshell/base/nsDocShell.cpp:9956
(Diff revision 2)
> +      SetCurrentURI(uri, nullptr, false, 0);

It is not quite clear to me what we're trying to do with this SetCurrentURI call. That isn't doing any loading.
I wonder if the setup should be something like, start loading about:blank here, and once it has been loaded, do the process switch?
Or would that cause flickering? Well, we could perhaps disable gfx updates for a moment or something

::: dom/ipc/ContentProcessManager.cpp:313
(Diff revision 2)
> +      if (tab->GetTabId() == aTabId) {

Hmm, is tabId unique even between processes? Could you verify that.

::: dom/ipc/TabParent.cpp:887
(Diff revision 2)
> +    // renderFrame = GetRenderFrame();

not sure what this is about.
Comment on attachment 8690739 [details]
MozReview Request: Bug 1209662 - Implement history navigation between processes r?smaug

(mozreview doesn't understand r-, and I need that so that I know which things I've reviewed)
Attachment #8690739 - Flags: review-
Attachment #8709905 - Flags: review?(bugs)
Comment on attachment 8709905 [details]
MozReview Request: Bug 1209662 - Add mochitest r?smaug

https://reviewboard.mozilla.org/r/31599/#review29477

So I think we really need to test pageshow/pagehide events, especially pagehide. I would expect the old page to get pagehide, but I think based on the new code in nsDocShell, we aren't getting that when the new page is about to be loaded.
Attachment #8709905 - Flags: review-
Hmm, or is the idea here that we don't actually do a normal pagehide? But just suspend the old process or something? But that would be rather surprising when going forward in session history. I'd expect pageshow events in that case.

(this all is a bit tricky, but maybe not as tricky as I thought)
Comment on attachment 8690732 [details]
MozReview Request: Bug 1209662 - Collect session history data for browser element, child side r?ttaubert,fabrice

https://reviewboard.mozilla.org/r/25921/#review31777
Attachment #8690732 - Flags: review?(fabrice) → review+
Comment on attachment 8690733 [details]
MozReview Request: Bug 1209662 - Collect session history data for browser element, parent side r?ttaubert,fabrice

https://reviewboard.mozilla.org/r/25923/#review31779
Attachment #8690733 - Flags: review?(fabrice) → review+
Attachment #8690734 - Flags: review?(fabrice) → review+
Comment on attachment 8690734 [details]
MozReview Request: Bug 1209662 - Restore session history data after switch process, parent side r?ttaubert,fabrice

https://reviewboard.mozilla.org/r/25925/#review31781

::: dom/browser-element/nsIBrowserElementAPI.idl:29
(Diff revision 2)
> -[scriptable, uuid(57758c10-6036-11e5-a837-0800200c9a66)]
> +[scriptable, uuid(f5322c3a-7f85-47a6-8711-882ecf5281d1)]

nit: no need to change uuids anymore
Comment on attachment 8690735 [details]
MozReview Request: Bug 1209662 - Restore session history data after switch process, child side r?ttaubert,fabrice

https://reviewboard.mozilla.org/r/25927/#review31783

r=me with nit addressed

::: dom/browser-element/BrowserElementChildSessionStore.js:21
(Diff revision 2)
> +                                  "resource://gre/modules/SessionStoreUtils.jsm");

that seems unused?
Attachment #8690735 - Flags: review?(fabrice) → review+
Do we need this for Fission, Nika?
Flags: needinfo?(nika)
I'm not sure. We need something like this in the future for fission, but I don't think that this particular implementation is the one we want to go with. 

Especially as the implementation is very likely to have bitrotted significantly since it was written 2 yrs ago.

I'll link this to bug 1445459 with see also, so that we know about their relationship to one-another.
Flags: needinfo?(nika)
See Also: → 1445459
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.