1294866 - Make the loading of favicon during SessionRestore use the correct originAttributes

Status: RESOLVED FIXED

(Core :: DOM: Security, defect, P1)

Description

[Steven Englehardt [:englehardt]]

Bug 1119386 largely fixed the issue of favicons loading with systemPrincipal instead of the document's principal. Session restore was intentionally left out of the fix [1], but is necessary as part of a broader effort to ensure favicon loads always use the correct originAttributes (See Bug 1277803). We have two approaches to a fix:

1. Serialize the documents principal for SessionRestore. See [2] and [3].
2. Fix Bug 1288054

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1119386#c56
[2] https://bugzilla.mozilla.org/show_bug.cgi?id=1277803#c8
[3] https://bugzilla.mozilla.org/show_bug.cgi?id=1119386#c55

Comment 1

[Tanvi Vyas[:tanvi]]

Should Session Restore really be causing any network requests?  Can't we use cached favicons?  If the favicons aren't cached, we could use a placeholder.  How often would we get a cache miss?  What are the reasons for getting a cache miss?

One potential reason for a cache miss would be if the user has set their browser to clear the cache / private data on restart.  In those cases, would session restore information be cleared / lost too?  Or do we still attempt to use session restore for users who ask us to clear data on restart?

Imagine I visit facebook.com at home, along with various other sites.  My browser crashes, and I decide to go to bed.  The next day I visit my work (or an internet cafe) and launch my browser.  I see the session restore page and decide not to restore my session.  I don't want to visit those pages while I'm working (or while I'm at an internet cafe on public wifi).  I go on to browse sites that are appropriate for the network I am on, thinking that the sites listed in session restore were never contacted.

However, this is not the case since a request for a favicon for facebook.com was made along with my facebook cookies.  This is bad because a) I'm on public wifi and my facebook cookies have just been leaked or b) I am not allowed to access facebook from work and I might get in trouble.

b) Is less likely with facebook (since facebook is embedded in a large percentage of the web).  But you could replace facebook with adult-content . com.

Who is the right person to comment on whether we can remove favicon network requests from session restore.  Gijs, Ehsan?

Comment 2

[:Gijs (he/him)]

(In reply to Tanvi Vyas - out 8/10, 8/11 [:tanvi] from comment #1)
> Should Session Restore really be causing any network requests?  Can't we use
> cached favicons?  If the favicons aren't cached, we could use a placeholder.

Marco is the best person to ask here, but he's away. It's not clear to me what the term 'session restore' means in this context. AIUI the discussion from bug 1119386 is about favicons that get loaded in the tabbrowser through session restore, IOW this only happens if a session is restored and we load favicons using the XUL image tags in the <tabbrowser> based on a URI we pass from session restore data, at which point we no longer have loading/triggering principal information for those loads.

Does that answer your question?

Comment 3

[Tanvi Vyas[:tanvi]]

Hmm, I imagined that the issue was during the load of about:sessionrestore, before the user decided if they wanted to restore.  There are favicons in that page.  If the user decides to restore, that's another story and of course we would make network request then.

Steve, if you know, can you clarify if this is on about:sessionrestore load or on load of the actual tabs that are being restored?

Comment 4

[Steven Englehardt [:englehardt]]

I was referring to the tabbrowser case (i.e. the two favicon loads that occur in [1]). During a session restore the loadingPrincipal is not available (see [2]). This bug is to fix that issue. Once this bug is fixed, Bug 1277803 will carry the loadingPrincipal to the subsequent XUL:image load.

I haven't checked to see if about:sessionrestore causes network requests and, if so, if these requests use the correct originAttributes.

[1] http://searchfox.org/mozilla-central/rev/b92ae9a263a05efefc0110eaad5e56369c9f1b10/browser/base/content/tabbrowser.xml#856
[2] http://searchfox.org/mozilla-central/rev/b92ae9a263a05efefc0110eaad5e56369c9f1b10/browser/components/sessionstore/SessionStore.jsm#818

Comment 5

[(no longer active)]

about:sessionrestore should never initiate a network connection: <https://dxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/content/aboutSessionRestore.js#81>

tabbrowser however will, and using the system principal for such loads is absolutely wrong.  At the very least, that means that the load will occur with the wrong OriginAttributes which can actually cause user visible breakages if the restored document had been loaded in a different container, for example.  Note that even if you want to rely on the favicon being cached, you need to have the right OriginAttributes in order to find the cached content.

The right fix here is to serialize the principal for the document in the sessionstore code, and use the serialized principal when setting the icon in tabbrowser.xml.

Comment 6

[Steven Englehardt [:englehardt]]

Attachment: 1294866-utils.patch

Moving methods to serialize and deserialize principals to a utility file.

Comment 7

[Steven Englehardt [:englehardt]]

Attachment: 1294866.patch

Adding an iconLoadingPrincipal field to the TabStatus data. I pull this attribute from the browser's contentPrincipal during any TabStatus updates.

I also add a serialized iconLoadingPrincipal attribute to the tab in this commit. This both provides an attribute to check in tests and will be consumed in Bug 1277803.

Comment 8

[Steven Englehardt [:englehardt]]

Attachment: 1294866-test.patch

Update tests to check the iconLoadingPrincipal attribute.

Comment 9

[Christoph Kerschbaumer [:ckerschb}]

Comment on attachment 8784619 [details] [diff] [review]
1294866-utils.patch

Review of attachment 8784619 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me, but I am not a browser peer. Billm knows a lot about favicons, he is probably the right reviewer.

Comment 10

[Christoph Kerschbaumer [:ckerschb}]

Comment on attachment 8784622 [details] [diff] [review]
1294866.patch

Review of attachment 8784622 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/tabbrowser.xml
@@ +863,5 @@
>              browser.mIconURL = aURI instanceof Ci.nsIURI ? aURI.spec : aURI;
> +            let loadingPrincipal = aLoadingPrincipal
> +              ? aLoadingPrincipal
> +              : Services.scriptSecurityManager.getSystemPrincipal();
> +            aTab.setAttribute("iconLoadingPrincipal", loadingPrincipal.origin);

Are you sure you want to use loadingPrincipal.origin? Or rather a serialization of the principal?

::: browser/components/sessionstore/TabState.jsm
@@ +126,5 @@
>  
> +    // Store the serialized contentPrincipal of this tab to use for image
> +    if (!("iconLoadingPrincipal" in tabData)) {
> +      let serialized = Utils.serializePrincipal(browser.contentPrincipal);
> +      tabData.iconLoadingPrincipal = serialized;

nit: no need for the tmp variable, just do:
tabData.iconLoadingPrincipal = Utils.serializePrincipal(browser.contentPrincipal);

Comment 11

[Tanvi Vyas[:tanvi]]

Today is Steve's last day, so he won't be able to finish this off.  Christoph's comments need addressing, then we need to push to try and flag billm for review.

Comment 12

[Steven Englehardt [:englehardt]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #10)
> Comment on attachment 8784622 [details] [diff] [review]
> 1294866.patch
> 
> Review of attachment 8784622 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/base/content/tabbrowser.xml
> @@ +863,5 @@
> >              browser.mIconURL = aURI instanceof Ci.nsIURI ? aURI.spec : aURI;
> > +            let loadingPrincipal = aLoadingPrincipal
> > +              ? aLoadingPrincipal
> > +              : Services.scriptSecurityManager.getSystemPrincipal();
> > +            aTab.setAttribute("iconLoadingPrincipal", loadingPrincipal.origin);
> 
> Are you sure you want to use loadingPrincipal.origin? Or rather a
> serialization of the principal?

The serialization isn't critical for this patch since TabState always serializes the browser's contentPrincipal rather than the Tab's iconLoadingPrincipal. This relies on the tab always having the correct contentPrincipal for the current favicon when it's TabState is updated. I believe this will be the case. If not, the iconLoadingPrincipal should be the full serialization and TabState should be updated to use that instead of contentPrincipal.

My motivation for using only the origin is that it matches what I will consume in Bug 1277803. Updating Bug 1277803 to consume the full principal object rather than just the origin is ideal, but I'm not sure that's possible.

Comment 13

[Tim Huang[:timhuang]]

Attachment: Part 1 : Move principal serialization to a utils file.

Update commit message. And flag billm for review.

Comment 14

[Tim Huang[:timhuang]]

Attachment: Storing the loadingPrincipal used to load favicons in SessionStore

Addressing Christoph's comments, and updating commit message.

Comment 15

[Tim Huang[:timhuang]]

Attachment: Part 3 : Tests to verify contentPrincipal persists during session restore.

Comment 16

[Tim Huang[:timhuang]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c8d98a06c191

Comment 17

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

Hi Tim, I'm really sorry but I don't think I'm going to be able to get to this until the end of next week at the earliest. I've been hit with a ton of reviews and I'm going to be at a work week next week. I also don't have much expertise with this code. Maybe Gijs would be a better reviewer?

Comment 18

[Tim Huang[:timhuang]]

Oh, Ok, I will flag Gijs to review. Thanks, billm.

Comment 19

[:Gijs (he/him)]

I'm out until Monday, and I expect Mike de Boer is a better reviewer for the session restore changes. Sorry for the repeated shifting of reviewer here...

Comment 20

[Mike de Boer [:mikedeboer]]

Comment on attachment 8785865 [details] [diff] [review]
Part 1 : Move principal serialization to a utils file.

Review of attachment 8785865 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with comments addressed.
Thanks for picking this up Tim!

::: browser/components/sessionstore/Utils.jsm
@@ +65,5 @@
> +  /**
> +   * Serialize principal data.
> +   *
> +   * @param principal.
> +   *        The principal to serialize.

nit: I want to slowly move to more standard JSDoc formatting - can you make this one line and add the datatype annotation here?

@@ +66,5 @@
> +   * Serialize principal data.
> +   *
> +   * @param principal.
> +   *        The principal to serialize.
> +   * @return The base64 encoded principal data.

nit: can you add the return datatype here? `{String}`

@@ +68,5 @@
> +   * @param principal.
> +   *        The principal to serialize.
> +   * @return The base64 encoded principal data.
> +   */
> +  serializePrincipal: function (principal) {

Please use the modern member definition:
```js
serializePrincipal(principal) {
  //...
},
```

@@ +69,5 @@
> +   *        The principal to serialize.
> +   * @return The base64 encoded principal data.
> +   */
> +  serializePrincipal: function (principal) {
> +    if (!principal) {

nit: no braces needed for single-statement if-clauses.

@@ +74,5 @@
> +      return null;
> +    }
> +
> +    let binaryStream = Cc["@mozilla.org/binaryoutputstream;1"].
> +                       createInstance(Ci.nsIObjectOutputStream);

nit: can you fix this to read:
```js
let binaryStream = Cc["@mozilla.org/binaryoutputstream;1"]
                     .createInstance(Ci.nsIObjectOutputStream);
```
?

@@ +83,5 @@
> +    binaryStream.close();
> +
> +    // Now we want to read the data from the pipe's input end and encode it.
> +    let scriptableStream = Cc["@mozilla.org/binaryinputstream;1"].
> +                           createInstance(Ci.nsIBinaryInputStream);

nit: same formatting problem here.

@@ +99,5 @@
> +   * Deserialize a base64 encoded principal (serialized with
> +   * Utils::serializePrincipal).
> +   *
> +   * @param principal_b64.
> +   *        A base64 encoded serialized principal.

nit: Can you add the datatype here and put the while thing on one line?

@@ +100,5 @@
> +   * Utils::serializePrincipal).
> +   *
> +   * @param principal_b64.
> +   *        A base64 encoded serialized principal.
> +   * @return A deserialized principal.

nit: can you add the datatype here?

@@ +102,5 @@
> +   * @param principal_b64.
> +   *        A base64 encoded serialized principal.
> +   * @return A deserialized principal.
> +   */
> +  deserializePrincipal: function (principal_b64) {

Please use the modern member definition:
```js
deserializePrincipal(principal_b64) {
  //...
},
```

@@ +103,5 @@
> +   *        A base64 encoded serialized principal.
> +   * @return A deserialized principal.
> +   */
> +  deserializePrincipal: function (principal_b64) {
> +    if (!principal_b64) {

nit: no braces needed for single-statement if-clauses.

@@ +106,5 @@
> +  deserializePrincipal: function (principal_b64) {
> +    if (!principal_b64) {
> +      return null;
> +    }
> +    var principal;

s/var/let/

@@ +108,5 @@
> +      return null;
> +    }
> +    var principal;
> +    var principalInput = Cc["@mozilla.org/io/string-input-stream;1"]
> +                             .createInstance(Ci.nsIStringInputStream);

nit: please fix the indentation of the second line here.

@@ +112,5 @@
> +                             .createInstance(Ci.nsIStringInputStream);
> +    var binaryData = atob(principal_b64);
> +    principalInput.setData(binaryData, binaryData.length);
> +    var binaryStream = Cc["@mozilla.org/binaryinputstream;1"].
> +                       createInstance(Ci.nsIObjectInputStream);

nit: please correct this, like the other cases above.

@@ +114,5 @@
> +    principalInput.setData(binaryData, binaryData.length);
> +    var binaryStream = Cc["@mozilla.org/binaryinputstream;1"].
> +                       createInstance(Ci.nsIObjectInputStream);
> +    binaryStream.setInputStream(principalInput);
> +    try { // Catch possible deserialization exceptions

nit: please put this comment on its own line. Or remove it, I don't think it's worth much.

@@ +116,5 @@
> +                       createInstance(Ci.nsIObjectInputStream);
> +    binaryStream.setInputStream(principalInput);
> +    try { // Catch possible deserialization exceptions
> +      principal = binaryStream.readObject(true);
> +    } catch (ex) { debug(ex); }

Please change this to say:
```js
} catch(ex) {
  Services.console.logStringMessage("SessionstoreUtils: " + ex);
}
```
(`debug` doesn't exist in this context)

Comment 21

[Mike de Boer [:mikedeboer]]

Comment on attachment 8785876 [details] [diff] [review]
Storing the loadingPrincipal used to load favicons in SessionStore

Review of attachment 8785876 [details] [diff] [review]:
-----------------------------------------------------------------

Looking good, but I'd like to take a last look at your change in tabbrowser.xml before landing.

::: browser/base/content/tabbrowser.xml
@@ +860,5 @@
>          <body>
>            <![CDATA[
>              let browser = this.getBrowserForTab(aTab);
> +            let serhelper = Cc["@mozilla.org/network/serialization-helper;1"]
> +                                 .getService(Ci.nsISerializationHelper);

Can you put this into <field> member? Check out `mURIFixup` as an example.

::: browser/components/sessionstore/SessionStore.jsm
@@ +813,5 @@
>          if ("image" in tabData) {
> +          // Use the serialized contentPrincipal with the new icon load
> +          let loadingPrincipal = Utils.deserializePrincipal(tabData.iconLoadingPrincipal);
> +          win.gBrowser.setIcon(tab, tabData.image, loadingPrincipal);
> +          TabStateCache.update(browser, {image: null, iconLoadingPrincipal: null});

nit: can you let the object literal breathe a bit? `{ image: null, ... }`

::: browser/components/sessionstore/TabAttributes.jsm
@@ +14,5 @@
>  // 'pending' is used internal by sessionstore and managed accordingly.
> +// 'iconLoadingPrincipal' is same as 'image' that it should be handled by
> +//                        using the gBrowser.getIcon()/setIcon() methods.
> +const ATTRIBUTES_TO_SKIP = new Set(["image", "muted", "pending",
> +                                    "iconLoadingPrincipal"]);

nit: you can keep this on one line, 80c is not a hard limit.

::: browser/components/sessionstore/TabState.jsm
@@ +123,5 @@
>        let tabbrowser = tab.ownerGlobal.gBrowser;
>        tabData.image = tabbrowser.getIcon(tab);
>      }
>  
> +    // Store the serialized contentPrincipal of this tab to use for image

nit: s/image/the icon/ and missing dot.

Comment 22

[Mike de Boer [:mikedeboer]]

Comment on attachment 8785865 [details] [diff] [review]
Part 1 : Move principal serialization to a utils file.

I just recently learned about nsISerializationHelper, by looking at the next patch, so can you make sure Utils.jsm uses that too?
All the stream magi used there is identical to nsSerializationHelper.cpp :-P

Please make it an XPCOMUtils.defineLazyServiceGetter.

Comment 23

[Mike de Boer [:mikedeboer]]

Comment on attachment 8785898 [details] [diff] [review]
Part 3 : Tests to verify contentPrincipal persists during session restore.

Review of attachment 8785898 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM, thanks! r=me with comments addressed.

::: browser/components/sessionstore/test/browser_attributes.js
@@ +16,5 @@
>    // Add a new tab with a nice icon.
>    let tab = gBrowser.addTab("about:robots");
>    yield promiseBrowserLoaded(tab.linkedBrowser);
>  
> +  // Check that the tab has an 'image' and 'iconLoadingPrincipal' attributes.

nit: -an

@@ +21,3 @@
>    ok(tab.hasAttribute("image"), "tab.image exists");
> +  ok(tab.hasAttribute("iconLoadingPrincipal"),
> +      "tab.iconLoadingPrincipal exists");

nit: 80c is not a hard limit.

@@ +33,4 @@
>    let {attributes} = JSON.parse(ss.getTabState(tab));
>    ok(!("image" in attributes), "'image' attribute not saved");
> +  ok(!("iconLoadingPrincipal" in attributes),
> +      "'iconLoadingPrincipal' attribute not saved");

nit: same here :)

::: browser/components/sessionstore/test/browser_label_and_icon.js
@@ +3,5 @@
>  
>  "use strict";
>  
> +let Cc = Components.classes;
> +let Ci = Components.interfaces;

nit: `const {classes: Cc, interfaces: Ci} = Components;`

@@ +42,5 @@
>    ok(gBrowser.getIcon(tab).startsWith("data:image/png;"), "icon is set");
>    is(tab.label, "Gort! Klaatu barada nikto!", "label is set");
>  
> +  let serhelper = Cc["@mozilla.org/network/serialization-helper;1"]
> +                       .getService(Ci.nsISerializationHelper);

nit: indentation.

@@ +47,5 @@
> +  let serializedPrincipal = tab.getAttribute("iconLoadingPrincipal");
> +  let iconLoadingPrincipal = serhelper.deserializeObject(serializedPrincipal)
> +                                      .QueryInterface(Ci.nsIPrincipal);
> +  is(iconLoadingPrincipal.origin, "about:robots",
> +      "correct loadingPrincipal used");

nit: 80c is not a hard limit.

Comment 24

[Tim Huang[:timhuang]]

Attachment: Part 1 : Move principal serialization to a utils file.

Addressing the review comments. Thanks, Mike.

Comment 25

[Tim Huang[:timhuang]]

Attachment: Part 2 : Storing the loadingPrincipal used to load favicons in SessionStore.

Comment 26

[Tim Huang[:timhuang]]

Attachment: Part 3 : Tests to verify contentPrincipal persists during session restore.

Comment 27

[Mike de Boer [:mikedeboer]]

Comment on attachment 8790277 [details] [diff] [review]
Part 1 : Move principal serialization to a utils file.

Review of attachment 8790277 [details] [diff] [review]:
-----------------------------------------------------------------

Looking excellent!

::: browser/components/sessionstore/Utils.jsm
@@ +88,5 @@
> +   * @return {nsIPrincipal} A deserialized principal.
> +   */
> +  deserializePrincipal(principal_b64) {
> +    if (!principal_b64)
> +      return null;

Maybe we should do `throw new Error("SessionStoreUtils: empty principal");`

Comment 28

[Mike de Boer [:mikedeboer]]

Comment on attachment 8790279 [details] [diff] [review]
Part 2 : Storing the loadingPrincipal used to load favicons in SessionStore.

Review of attachment 8790279 [details] [diff] [review]:
-----------------------------------------------------------------

Looks OK to me, but I have a question:

::: browser/base/content/tabbrowser.xml
@@ +883,2 @@
>                  aTab.setAttribute("image", sizedIconUrl);
> +                aTab.setAttribute("iconLoadingPrincipal",

If we're skipping this attribute in TabAttributes.jsm, why do you care about setting it here? What does this do?

Comment 29

[Tim Huang[:timhuang]]

(In reply to Mike de Boer [:mikedeboer] from comment #28)
> Comment on attachment 8790279 [details] [diff] [review]
> Part 2 : Storing the loadingPrincipal used to load favicons in SessionStore.
> 
> Review of attachment 8790279 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks OK to me, but I have a question:
> 
> ::: browser/base/content/tabbrowser.xml
> @@ +883,2 @@
> >                  aTab.setAttribute("image", sizedIconUrl);
> > +                aTab.setAttribute("iconLoadingPrincipal",
> 
> If we're skipping this attribute in TabAttributes.jsm, why do you care about
> setting it here? What does this do?

This "iconLoadingPrincipal" will be used as the loading principal of the favicon loading in the Bug 1277803. But here, I think this attribute is used for checking the loading principal of the restored favicon that we can make sure the loading principal is correct in browser_label_and_icon.js.

Comment 30

[Mike de Boer [:mikedeboer]]

(In reply to Tim Huang[:timhuang] from comment #29)
> This "iconLoadingPrincipal" will be used as the loading principal of the
> favicon loading in the Bug 1277803. But here, I think this attribute is used
> for checking the loading principal of the restored favicon that we can make
> sure the loading principal is correct in browser_label_and_icon.js.

Well, I get that you want to test this, but isn't it relatively expensive to serialize objects this often when opening a tab -> setIcon?
I also don't see an update to browser_label_and_icon.js, so it looks like this new attribute is used nowhere.

Comment 31

[Tim Huang[:timhuang]]

(In reply to Mike de Boer [:mikedeboer] from comment #30)
> (In reply to Tim Huang[:timhuang] from comment #29)
> > This "iconLoadingPrincipal" will be used as the loading principal of the
> > favicon loading in the Bug 1277803. But here, I think this attribute is used
> > for checking the loading principal of the restored favicon that we can make
> > sure the loading principal is correct in browser_label_and_icon.js.
> 
> Well, I get that you want to test this, but isn't it relatively expensive to
> serialize objects this often when opening a tab -> setIcon?

The setIcon is a one-time job when opening a tab, we won't change the favicon frequently, so I think it's not a big performance issue. Besides, we need to serialize the principal not only for the testing purpose here but also for making favicon loading obeys the originAttributes in Bug 1277803. 

> I also don't see an update to browser_label_and_icon.js, so it looks like
> this new attribute is used nowhere.

The browser_label_and_icon.js is changed at the 'Part: 3' patch. [1]

[1] https://bugzilla.mozilla.org/attachment.cgi?id=8790280&action=diff#a/browser/components/sessionstore/test/browser_label_and_icon.js_sec3

Comment 32

[:Gijs (he/him)]

(In reply to Tim Huang[:timhuang] from comment #31)
> (In reply to Mike de Boer [:mikedeboer] from comment #30)
> > (In reply to Tim Huang[:timhuang] from comment #29)
> > > This "iconLoadingPrincipal" will be used as the loading principal of the
> > > favicon loading in the Bug 1277803. But here, I think this attribute is used
> > > for checking the loading principal of the restored favicon that we can make
> > > sure the loading principal is correct in browser_label_and_icon.js.
> > 
> > Well, I get that you want to test this, but isn't it relatively expensive to
> > serialize objects this often when opening a tab -> setIcon?
> 
> The setIcon is a one-time job when opening a tab, we won't change the
> favicon frequently, so I think it's not a big performance issue.

It is not uncommon for sites to specify more than one icon (for different resolutions), or to change the icon to notify the user of some kind of change on the page. So "one time" is not accurate. It's true that this isn't "hot" code, though.

> Besides, we
> need to serialize the principal not only for the testing purpose here but
> also for making favicon loading obeys the originAttributes in Bug 1277803. 

Right, but the loading principal of the load doesn't change if 'only' the favicon changes, right? It's still going to be the same as before. That might be optimizable if the serializing and deserializing of the principal is very expensive (ie we could send a setIcon message without a principal from the content process if the principal was the same as the previous load)?

Comment 33

[Mike de Boer [:mikedeboer]]

(In reply to :Gijs Kruitbosch from comment #32)
> Right, but the loading principal of the load doesn't change if 'only' the
> favicon changes, right? It's still going to be the same as before. That
> might be optimizable if the serializing and deserializing of the principal
> is very expensive (ie we could send a setIcon message without a principal
> from the content process if the principal was the same as the previous load)?

I'd be fine with the following solution: in the most common case, the principal will be the system principal. How about we set the attribute value to 'system' instead of serializing the same object over and over again?

Comment 34

[:Gijs (he/him)]

(In reply to Mike de Boer [:mikedeboer] from comment #33)
> (In reply to :Gijs Kruitbosch from comment #32)
> > Right, but the loading principal of the load doesn't change if 'only' the
> > favicon changes, right? It's still going to be the same as before. That
> > might be optimizable if the serializing and deserializing of the principal
> > is very expensive (ie we could send a setIcon message without a principal
> > from the content process if the principal was the same as the previous load)?
> 
> I'd be fine with the following solution: in the most common case, the
> principal will be the system principal.

Uhhh, really? I really hope this is not true. The "point" is that we should be loading these favicons based on the permissions of the page itself, and so very much not based on a 'system' principal.

Comment 35

[Mike de Boer [:mikedeboer]]

(In reply to :Gijs Kruitbosch from comment #34)
> Uhhh, really? I really hope this is not true. The "point" is that we should
> be loading these favicons based on the permissions of the page itself, and
> so very much not based on a 'system' principal.

Ah yes, my bad.

Comment 36

[Tim Huang[:timhuang]]

(In reply to :Gijs Kruitbosch from comment #32)
> (In reply to Tim Huang[:timhuang] from comment #31)
> > (In reply to Mike de Boer [:mikedeboer] from comment #30)
> > > (In reply to Tim Huang[:timhuang] from comment #29)
> > > > This "iconLoadingPrincipal" will be used as the loading principal of the
> > > > favicon loading in the Bug 1277803. But here, I think this attribute is used
> > > > for checking the loading principal of the restored favicon that we can make
> > > > sure the loading principal is correct in browser_label_and_icon.js.
> > > 
> > > Well, I get that you want to test this, but isn't it relatively expensive to
> > > serialize objects this often when opening a tab -> setIcon?
> > 
> > The setIcon is a one-time job when opening a tab, we won't change the
> > favicon frequently, so I think it's not a big performance issue.
> 
> It is not uncommon for sites to specify more than one icon (for different
> resolutions), or to change the icon to notify the user of some kind of
> change on the page. So "one time" is not accurate. It's true that this isn't
> "hot" code, though.
> 
> > Besides, we
> > need to serialize the principal not only for the testing purpose here but
> > also for making favicon loading obeys the originAttributes in Bug 1277803. 
> 
> Right, but the loading principal of the load doesn't change if 'only' the
> favicon changes, right? It's still going to be the same as before. That
> might be optimizable if the serializing and deserializing of the principal
> is very expensive (ie we could send a setIcon message without a principal
> from the content process if the principal was the same as the previous load)?

I think we could store the loading principal in the browser object. When every time setIcon has been called with the aLoadingPrincipal, we could compare the one in the browser object with aLoadingPrincipal by using nsIPrincipal.equals(). We serialize the principal if and only if they are different.

Comment 37

[Mike de Boer [:mikedeboer]]

(In reply to Tim Huang[:timhuang] from comment #36)
> I think we could store the loading principal in the browser object. When
> every time setIcon has been called with the aLoadingPrincipal, we could
> compare the one in the browser object with aLoadingPrincipal by using
> nsIPrincipal.equals(). We serialize the principal if and only if they are
> different.

SGTM!

Comment 38

[Tim Huang[:timhuang]]

Attachment: Part 2 : Storing the loadingPrincipal used to load favicons in SessionStore.

Comment 39

[Mike de Boer [:mikedeboer]]

Comment on attachment 8790701 [details] [diff] [review]
Part 2 : Storing the loadingPrincipal used to load favicons in SessionStore.

Review of attachment 8790701 [details] [diff] [review]:
-----------------------------------------------------------------

/me happy! Thanks Tim ;)
r=me with the one comment addressed.

::: browser/base/content/tabbrowser.xml
@@ +892,3 @@
>                  aTab.removeAttribute("image");
> +                aTab.removeAttribute("iconLoadingPrincipal");
> +                delete browser.mIconLoadingPrincipal;

Please add a new field `mIconLoadingPrincipal`, initialized to `null` (just like `mCurrentTab`).

Comment 40

[Tim Huang[:timhuang]]

Attachment: Part 2 : Storing the loadingPrincipal used to load favicons in SessionStore.

Thanks, mike. Addressing your review comment.

Comment 41

[Tim Huang[:timhuang]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fce13d38618d

Comment 42

[Tim Huang[:timhuang]]

Try looks good.

Comment 43

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/91be005ab8e6
Part 1: Move principal serialization to a utils file. r=mikedeboer
https://hg.mozilla.org/integration/mozilla-inbound/rev/dafa28412032
Part 2: Storing the loadingPrincipal used to load favicons in SessionStore. r=mikedeboer
https://hg.mozilla.org/integration/mozilla-inbound/rev/8639daa019b6
Part 3: Tests to verify contentPrincipal persists during session restore. r=mikedeboer

Comment 44

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/91be005ab8e6
https://hg.mozilla.org/mozilla-central/rev/dafa28412032
https://hg.mozilla.org/mozilla-central/rev/8639daa019b6