Magic-Mirror/gecko-dev
1
0
Fork 0
mirror of https://github.com/mozilla/gecko-dev.git synced 2024-10-09 11:25:00 +00:00
Code Issues Actions Packages Projects Releases Wiki Activity

Bug 1488296 - Race condition when setting favicons for a browser with a changed currentURI. r=mossop

Browse Source 
There is a race condition between the time we decide to fetch an icon and the time we actually store that icon, where the original browser currentURI may have changed.

Differential Revision: https://phabricator.services.mozilla.com/D5685

--HG--
extra : moz-landing-system : lando
This commit is contained in:
Marco Bonardo 2018-09-13 13:24:41 +00:00
parent 3da37fa945
commit 751325b9aa
5 changed files with 62 additions and 63 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

10
browser/actors/LinkHandlerChild.jsm
View File

@ -34,10 +34,10 @@ class LinkHandlerChild extends ActorChild {
Services.prefs.getBoolPref("browser.chrome.site_icons", true)) {
// Inject the default icon. Use documentURIObject so that we do the right
// thing with about:-style error pages. See bug 453442
let baseURI = this.content.document.documentURIObject;
if (["http", "https"].includes(baseURI.scheme)) {
let pageURI = this.content.document.documentURIObject;
if (["http", "https"].includes(pageURI.scheme)) {
this.seenTabIcon = true;
this.iconLoader.addDefaultIcon(baseURI);
this.iconLoader.addDefaultIcon(pageURI);
}
}
}
@ -137,13 +137,11 @@ class LinkHandlerChild extends ActorChild {
return;
}
let iconInfo = FaviconLoader.makeFaviconFromLink(link, isRichIcon);
if (iconInfo) {
if (this.iconLoader.addIconFromLink(link, isRichIcon)) {
iconAdded = true;
if (!isRichIcon) {
this.seenTabIcon = true;
}
this.iconLoader.addIcon(iconInfo);
}
break;
case "search":

12
browser/base/content/browser.js
View File

@ -3666,7 +3666,8 @@ const DOMEventHandler = {
break;
case "Link:SetIcon":
this.setIconFromLink(aMsg.target, aMsg.data.originalURL, aMsg.data.canUseForTab,
this.setIconFromLink(aMsg.target, aMsg.data.pageURL,
aMsg.data.originalURL, aMsg.data.canUseForTab,
aMsg.data.expiration, aMsg.data.iconURL);
break;
@ -3704,14 +3705,15 @@ const DOMEventHandler = {
tab.removeAttribute("pendingicon");
},
setIconFromLink(aBrowser, aOriginalURL, aCanUseForTab, aExpiration, aIconURL) {
setIconFromLink(aBrowser, aPageURL, aOriginalURL, aCanUseForTab, aExpiration, aIconURL) {
let tab = gBrowser.getTabForBrowser(aBrowser);
if (!tab)
if (!tab) {
return false;
}
try {
PlacesUIUtils.loadFavicon(aBrowser, Services.scriptSecurityManager.getSystemPrincipal(),
makeURI(aOriginalURL), aExpiration, makeURI(aIconURL));
makeURI(aPageURL), makeURI(aOriginalURL),
aExpiration, makeURI(aIconURL));
} catch (ex) {
Cu.reportError(ex);
}

32
browser/components/originattributes/test/browser/browser_favicon_userContextId.js
View File

@ -2,15 +2,15 @@
* Bug 1277803 - A test caes for testing favicon loading across different userContextId.
*/
const CC = Components.Constructor;
ChromeUtils.defineModuleGetter(this, "Promise",
"resource://gre/modules/Promise.jsm");
let EventUtils = {};
Services.scriptloader.loadSubScript(
"chrome://mochikit/content/tests/SimpleTest/EventUtils.js", EventUtils);
ChromeUtils.defineModuleGetter(this, "PromiseUtils",
"resource://gre/modules/PromiseUtils.jsm");
ChromeUtils.defineModuleGetter(this, "PlacesTestUtils",
"resource://testing-common/PlacesTestUtils.jsm");
const TEST_SITE = "http://example.net";
const TEST_THIRD_PARTY_SITE = "http://mochi.test:8888";
@ -27,7 +27,6 @@ const USER_CONTEXT_ID_PERSONAL = 1;
const USER_CONTEXT_ID_WORK = 2;
let systemPrincipal = Services.scriptSecurityManager.getSystemPrincipal();
let makeURI = ChromeUtils.import("resource://gre/modules/BrowserUtils.jsm", {}).BrowserUtils.makeURI;
function clearAllImageCaches() {
var tools = SpecialPowers.Cc["@mozilla.org/image/tools;1"]
@ -110,7 +109,7 @@ FaviconObserver.prototype = {
this._expectedPrincipal = Services.scriptSecurityManager
.createCodebasePrincipal(aPageURI, { userContextId: aUserContextId });
this._faviconURL = aFaviconURL;
this._faviconLoaded = new Promise.defer();
this._faviconLoaded = PromiseUtils.defer();
},
get promise() {
@ -119,20 +118,11 @@ FaviconObserver.prototype = {
};
function waitOnFaviconLoaded(aFaviconURL) {
return new Promise(resolve => {
let observer = {
onPageChanged(uri, attr, value, id) {
if (attr === Ci.nsINavHistoryObserver.ATTRIBUTE_FAVICON &&
value === aFaviconURL) {
resolve();
PlacesUtils.history.removeObserver(observer, false);
}
},
};
PlacesUtils.history.addObserver(observer);
});
return PlacesTestUtils.waitForNotification(
"onPageChanged",
(uri, attr, value, id) => attr === Ci.nsINavHistoryObserver.ATTRIBUTE_FAVICON &&
value === aFaviconURL,
"history");
}
async function generateCookies(aHost) {

15
browser/components/places/PlacesUIUtils.jsm
View File

@ -163,9 +163,9 @@ let InternalFaviconLoader = {
});
},
loadFavicon(browser, principal, uri, expiration, iconURI) {
loadFavicon(browser, principal, pageURI, uri, expiration, iconURI) {
this.ensureInitialized();
let win = browser.ownerGlobal;
let {ownerGlobal: win, innerWindowID} = browser;
if (!gFaviconLoadDataMap.has(win)) {
gFaviconLoadDataMap.set(win, []);
let unloadHandler = event => {
@ -179,8 +179,6 @@ let InternalFaviconLoader = {
win.addEventListener("unload", unloadHandler, true);
}
let {innerWindowID, currentURI} = browser;
// First we do the actual setAndFetch call:
let loadType = PrivateBrowsingUtils.isWindowPrivate(win)
? PlacesUtils.favicons.FAVICON_LOAD_PRIVATE
@ -193,8 +191,8 @@ let InternalFaviconLoader = {
expiration, principal);
}
let request = PlacesUtils.favicons.setAndFetchFaviconForPage(currentURI, uri, false,
loadType, callback, principal);
let request = PlacesUtils.favicons.setAndFetchFaviconForPage(
pageURI, uri, false, loadType, callback, principal);
// Now register the result so we can cancel it if/when necessary.
if (!request) {
@ -313,15 +311,16 @@ var PlacesUIUtils = {
* set and fetch a favicon. Can only be used from the parent process.
* @param browser {Browser} The XUL browser element for which we're fetching a favicon.
* @param principal {Principal} The loading principal to use for the fetch.
* @pram pageURI {URI} The page URI associated to this favicon load.
* @param uri {URI} The URI to fetch.
* @param expiration {Number} An optional expiration time.
* @param iconURI {URI} An optional data: URI holding the icon's data.
*/
loadFavicon(browser, principal, uri, expiration = 0, iconURI = null) {
loadFavicon(browser, principal, pageURI, uri, expiration = 0, iconURI = null) {
if (gInContentProcess) {
throw new Error("Can't track loads from within the child process!");
}
InternalFaviconLoader.loadFavicon(browser, principal, uri, expiration, iconURI);
InternalFaviconLoader.loadFavicon(browser, principal, pageURI, uri, expiration, iconURI);
},
/**

56
browser/modules/FaviconLoader.jsm
View File

@ -377,6 +377,7 @@ class IconLoader {
if (LOCAL_FAVICON_SCHEMES.includes(iconInfo.iconUri.scheme)) {
this.mm.sendAsyncMessage("Link:SetIcon", {
pageURL: iconInfo.pageUri.spec,
originalURL: iconInfo.iconUri.spec,
canUseForTab: !iconInfo.isRichIcon,
expiration: undefined,
@ -396,6 +397,7 @@ class IconLoader {
let { dataURL, expiration } = await this._loader.load();
this.mm.sendAsyncMessage("Link:SetIcon", {
pageURL: iconInfo.pageUri.spec,
originalURL: iconInfo.iconUri.spec,
canUseForTab: !iconInfo.isRichIcon,
expiration,
@ -453,21 +455,28 @@ class FaviconLoader {
}
}
addIcon(iconInfo) {
this.iconInfos.push(iconInfo);
this.iconTask.arm();
addIconFromLink(aLink, aIsRichIcon) {
let iconInfo = makeFaviconFromLink(aLink, aIsRichIcon);
if (iconInfo) {
this.iconInfos.push(iconInfo);
this.iconTask.arm();
return true;
}
return false;
}
addDefaultIcon(baseURI) {
addDefaultIcon(pageUri) {
// Currently ImageDocuments will just load the default favicon, see bug
// 403651 for discussion.
this.addIcon({
iconUri: baseURI.mutate().setPathQueryRef("/favicon.ico").finalize(),
this.iconInfos.push({
pageUri,
iconUri: pageUri.mutate().setPathQueryRef("/favicon.ico").finalize(),
width: -1,
isRichIcon: false,
type: TYPE_ICO,
node: this.mm.content.document,
});
this.iconTask.arm();
}
onPageShow() {
@ -485,21 +494,22 @@ class FaviconLoader {
this.iconTask.disarm();
this.iconInfos = [];
}
static makeFaviconFromLink(aLink, aIsRichIcon) {
let iconUri = getLinkIconURI(aLink);
if (!iconUri)
return null;
// Extract the size type and width.
let width = extractIconSize(aLink.sizes);
return {
iconUri,
width,
isRichIcon: aIsRichIcon,
type: aLink.type,
node: aLink,
};
}
}
function makeFaviconFromLink(aLink, aIsRichIcon) {
let iconUri = getLinkIconURI(aLink);
if (!iconUri)
return null;
// Extract the size type and width.
let width = extractIconSize(aLink.sizes);
return {
pageUri: aLink.ownerDocument.documentURIObject,
iconUri,
width,
isRichIcon: aIsRichIcon,
type: aLink.type,
node: aLink,
};
}