Closed Bug 1452666 Opened 7 years ago Closed 7 years ago

Navigations initiated by an extensions content script (with an expanded principal) cause error in nsISerializationHelper.serializeToString

Categories

(Firefox :: Session Restore, defect, P1)

Product:
Firefox
Component:
Session Restore
Version:
59 Branch
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 62
Tracking Flags:
Tracking Status
firefox-esr52 --- unaffected
firefox-esr60 62+ verified
firefox60 + wontfix
firefox61 + verified
firefox62 + verified

People

(Reporter: robwu, Assigned: jkt)

References

(Blocks 1 open bug)

Details

(Keywords: regression)

Attachments

(3 files, 2 obsolete files)

sessionstore-principal-error.zip
759 bytes, application/zip
Details
Bug 1452666 - Simplify SessionHistory serialization code and test expanded principals are serialized and can restore tabs.
59 bytes, text/x-review-board-request
bzbarsky
: review+
mikedeboer
: review+
RyanVM
: approval-mozilla-esr60+
Details
Implement nsISerializable on expanded principals
9.40 KB, patch
kmag
: review+
RyanVM
: approval-mozilla-beta+
Details | Diff | Splinter Review
STR: 1. Visit about:debugging and load the attached extension, which opens example.com. 2. Click on the extension button (in the browser UI) to trigger a navigation from the extension's content script. 3. Look at the global JS console. Expected result: No errors Actual result : The following error is shown: Utils: Failed to serialize principal '[xpconnect wrapped nsIPrincipal]' [Exception... "Could not convert JavaScript argument arg 0 [nsISerializationHelper.serializeToString]" nsresult: "0x80570009 (NS_ERROR_XPC_BAD_CONVERT_JS)" location: "JS frame :: resource://gre/modules/sessionstore/Utils.jsm :: serializePrincipal :: line 126" data: no] Tip: To debug, attach a content toolbox, and put a conditional breakpoint at https://searchfox.org/mozilla-central/rev/7ccb618f45a1398e31a086a009f87c8fd3a790b6/toolkit/modules/sessionstore/Utils.jsm#122 with condition: principal.origin.startsWith('[Expanded Principal') Stack trace: serializePrincipal@resource://gre/modules/sessionstore/Utils.jsm:122:9 serializeEntry@resource://gre/modules/sessionstore/SessionHistory.jsm:234:35 collect@resource://gre/modules/sessionstore/SessionHistory.jsm:91:21 collect@resource://gre/modules/sessionstore/SessionHistory.jsm:30:12 collectFrom/<@chrome://browser/content/content-sessionStore.js:384:21 send@chrome://browser/content/content-sessionStore.js:897:19 sendWhenIdle@chrome://browser/content/content-sessionStore.js:860:9 requestIdleCallback handler*sendWhenIdle@chrome://browser/content/content-sessionStore.js:868:7 push/this._timeout<@chrome://browser/content/content-sessionStore.js:840:15 notify@resource://gre/modules/Timer.jsm:44:7 This is probably the root cause of bug 1429490, which was "fixed" by a try-catch. I have reproduced the problem in Firefox 59.0.2 and the Firefox Nightly (61.0a1, build ID 20180407220122).
It seems that the user-visible effect of this is that session restoration for the tab fails. After running the above STR, close the tab and use Ctrl-Shift-T to restore the tab. Then observe that the tab content is greyish and seemingly broken: - Refresh button is greyed out, and the F5 key does not work either. - Back button is clickable, but clicking does not trigger a navigation, but an error in the console: NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIWebNavigation.goBack] - Right-click inside the tab does not activate a context menu.
Christoph, would you be able to take a look at this or find someone who can? I don't have the required knowledge - I think - to find the best solution for this... Rob, thanks for the detailed STR - I didn't realize the seriousness of this bug until I took a second look now. Please feel free to ping/ nag or anything to escalate an issue like this in the future!
Blocks: ss-reliability
Severity: normal → critical
Flags: needinfo?(ckerschb)
Priority: -- → P1
(In reply to Mike de Boer [:mikedeboer] from comment #3) > Christoph, would you be able to take a look at this or find someone who can? > I don't have the required knowledge - I think - to find the best solution > for this... Baku offered to take a look - thanks baku!
Flags: needinfo?(ckerschb) → needinfo?(amarchesini)
Depends on: 1449301
It appears we have an expanded principal of the Extension URL and also the page. I suspect we need to be resolving the triggering principal to be the page earlier than this code is running, that way we can serialize the principal into history. I spoke to baku about this, I'm looking into this as part of my other work.
Flags: needinfo?(amarchesini)
So LoadURI in DocShell is loading with a ExpandedPrincipal of both the extension URL and the tab it's opening. The principal to inherit gets set correctly however by the time we serialize to the session history we don't have a triggeringPrincipal that we can use. I don't know if this is an issue with the design of how this code loads however the patch fixes the issue reported here such that it is able to load by using the principal to inherit. Bz would you accept the patch with a decent test or should a different approach be taken?
Flags: needinfo?(bzbarsky)
Assignee: nobody → jkt
I'm looking into why debug builds running verify are leaking windows.
Comment on attachment 8979025 [details] Bug 1452666 - Simplify SessionHistory serialization code and test expanded principals are serialized and can restore tabs. https://reviewboard.mozilla.org/r/245296/#review251848 I'd really like to take a peek at the next version. Thanks for working on this! ::: browser/components/extensions/test/browser/browser_ext_sessions_restoreTab.js:5 (Diff revision 4) > +/* -*- Mode: indent-tabs-mode: nil; js-indent-level: 2 -*- */ > +/* vim: set sts=2 sw=2 et tw=80: */ > +"use strict"; > + > +SimpleTest.requestCompleteLog(); What does this do? ::: browser/components/extensions/test/browser/browser_ext_sessions_restoreTab.js:49 (Diff revision 4) > + let win = await BrowserTestUtils.openNewBrowserWindow({}); > + > + await extension.startup(); > + > + const contentScriptTabURL = "http://example.com/?fromExt"; > + // Open and close a tabs nit: missing dot. ::: browser/components/extensions/test/browser/browser_ext_sessions_restoreTab.js:75 (Diff revision 4) > + // Close the window. > + await BrowserTestUtils.closeWindow(win); > +}); > + > + > +// Check that we can restore a tab modified by an extension nit: missing dot. ::: toolkit/modules/sessionstore/SessionHistory.jsm:227 (Diff revision 4) > + return serializedPrincipal; > + } > + > + // Collect triggeringPrincipal data for the current history entry. > + if (shEntry.principalToInherit) { > + entry.principalToInherit_base64 = tryAndSerializePrincipal(shEntry.principalToInherit); `Utils.serializePrincipal()` already has this try...catch handling logic, thus it never throws: https://searchfox.org/mozilla-central/source/toolkit/modules/sessionstore/Utils.jsm#120 In other words: can you remove the `tryAndSerializePrincipal` function and replace its uses with `Utils.serializePrincipal`? ::: toolkit/modules/sessionstore/SessionHistory.jsm:231 (Diff revision 4) > - try { > - let triggeringPrincipal = Utils.serializePrincipal(shEntry.triggeringPrincipal); > + if (shEntry.triggeringPrincipal.isExpandedPrincipal) { > + if (entry.principalToInherit_base64) { To be clear: this doesn't need to be ```js if (shEntry.triggeringPrincipal.isExpandedPrincipal && entry.principalToInherit_base64) { //... } else { //... } ``` instead to make sure that the fallback is always there?
Attachment #8979025 - Flags: review?(mdeboer)
Comment on attachment 8979025 [details] Bug 1452666 - Simplify SessionHistory serialization code and test expanded principals are serialized and can restore tabs. https://reviewboard.mozilla.org/r/245296/#review251848 > What does this do? Not entirely sure, should have removed. > `Utils.serializePrincipal()` already has this try...catch handling logic, thus it never throws: https://searchfox.org/mozilla-central/source/toolkit/modules/sessionstore/Utils.jsm#120 > > In other words: can you remove the `tryAndSerializePrincipal` function and replace its uses with `Utils.serializePrincipal`? I think this can be removed, looks like this try code was added in sept 2016 before the change to the Util. Thanks for the heads up. > To be clear: this doesn't need to be > > ```js > if (shEntry.triggeringPrincipal.isExpandedPrincipal && entry.principalToInherit_base64) { > //... > } else { > //... > } > ``` > instead to make sure that the fallback is always there? I mean I don't think the fallback will work (both would be null), either way I think both will be broken. The alternative could be serialising something else (can we serialize a null principal perhaps?). However I think your code makes it easier to read.
Sorry for the lag here; most of this stuff happened over the weekend.... The triggering principal for a load can be an expanded principal. This is, as you note, different from the principal to inherit and is needed to do the load correctly, in general.
Flags: needinfo?(bzbarsky)
Comment on attachment 8979025 [details] Bug 1452666 - Simplify SessionHistory serialization code and test expanded principals are serialized and can restore tabs. https://reviewboard.mozilla.org/r/245296/#review251904 ::: toolkit/modules/sessionstore/SessionHistory.jsm:223 (Diff revision 5) > } > > if (shEntry.triggeringPrincipal) { > - try { > - let triggeringPrincipal = Utils.serializePrincipal(shEntry.triggeringPrincipal); > - if (triggeringPrincipal) { > + if (shEntry.triggeringPrincipal.isExpandedPrincipal && > + entry.principalToInherit_base64) { > + entry.triggeringPrincipal_base64 = entry.principalToInherit_base64; This doesn't look to me like it would give the right behavior, in general. In particular, various security checks done on the load wouldn't work correctly with this change, I would think...
Attachment #8979025 - Flags: review?(bzbarsky) → review-
It seems to me that the right fix would be to make expanded principals serializable. All other sorts of principals are; why should these be different?
Expanded principals are supposed to be serializable. Bill fixed the bugs with serializing them a few years ago. But, unfortunately, we have multiple ways of serializing principals, and not all of them work equally well. I know Mossop and some people he was working with ran into this a few weeks ago. I agree that the correct solution here is to fix serialization of expanded principals in this case, ideally by rearranging things so we use the same serialization process everywhere.
> Bill fixed the bugs with serializing them a few years ago For IPC. Session store is using nsISerializable serialization, and for that we have: NS_IMETHODIMP ExpandedPrincipal::Write(nsIObjectOutputStream* aStream) { return NS_ERROR_NOT_IMPLEMENTED; } and it doesn't QI to nsISerializable. Seems like we should just write out mPrincipals.Length() and then concatenate the serializations of the principals in it, and the reverse for Read().
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #20) > > Bill fixed the bugs with serializing them a few years ago > > For IPC. Session store is using nsISerializable serialization, and for that > we have: Yup, hence my comment "unfortunately, we have multiple ways of serializing principals, and not all of them work equally well ... ideally by rearranging things so we use the same serialization process everywhere"
jkt, does that patch fix things, perchance?
Flags: needinfo?(jkt)
> Expanded principals are supposed to be serializable. Bill fixed the bugs with serializing them a few years ago. Oh really, I was told we never want to serialize expanded. I will check over the patch tomorrow if it fixes the issue, thanks bz! > why should these be different? I heard we resolve the expanded where possible and by the time they are used they should be standard principals. We might want to double check with bholley and ckerschb if it is now safe to serialize expanded.
(In reply to Jonathan Kingston [:jkt] from comment #24) > > why should these be different? > > I heard we resolve the expanded where possible and by the time they are used > they should be standard principals. > We might want to double check with bholley and ckerschb if it is now safe to > serialize expanded. Depending on what you mean by "resolve", we only do that when choosing principals to inherit. Expanded principals are still perfectly valid in other contexts.
I got the following error trying to run the code: Utils: Failed to deserialize principal_b64 '6O6IsFVxQIakWzmnFpBr2wAAAAAAAAAAwAAAAAAAAEYAAAABAAAAAmU+Dk0+5EX6snKXwgvAHrgAAAAAAAAAAMAAAAAAAABGAd6UctCANBHTk5kAEEug/UCSBzpUbXhPMJE6uHGBMgjGAAAAAv////8AAAG7AQAAAEJodHRwczovL2J1Z3ppbGxhLm1vemlsbGEub3JnL2J1Z2xpc3QuY2dpP3F1aWNrc2VhcmNoPXN0cmFuZ2UrdGhpbmcAAAAAAAAABQAAAAgAAAAUAAAACP////8AAAAI/////wAAAAgAAAAUAAAAHAAAACYAAAAcAAAADAAAABwAAAABAAAAHQAAAAcAAAAlAAAAAwAAAAD/////AAAAKQAAABkAAAAd/////wEAAAAAAAAAAAAAAAEJ2e0a5dRABL/gJ865I9mss8TArr1eTK2H4I0hDbs/nwHelHLQgDQR05OZABBLoP1Akgc6VG14TzCROrhxgTIIxgAAAAL/////AAABuwEAAABCaHR0cHM6Ly9idWd6aWxsYS5tb3ppbGxhLm9yZy9idWdsaXN0LmNnaT9xdWlja3NlYXJjaD1zdHJhbmdlK3RoaW5nAAAAAAAAAAUAAAAIAAAAFAAAAAj/////AAAACP////8AAAAIAAAAFAAAABwAAAAmAAAAHAAAAAwAAAAcAAAAAQAAAB0AAAAHAAAAJQAAAAMAAAAA/////wAAACkAAAAZAAAAHf////8BAAAAAAAAAAAAAQAAAosAZABlAGYAYQB1AGwAdAAtAHMAcgBjACAAaAB0AHQAcABzADoALwAvAGIAdQBnAHoAaQBsAGwAYQAuAG0AbwB6AGkAbABsAGEALgBvAHIAZwA7ACAAdwBvAHIAawBlAHIALQBzAHIAYwAgACcAbgBvAG4AZQAnADsAIABjAG8AbgBuAGUAYwB0AC0AcwByAGMAIABoAHQAdABwAHMAOgAvAC8AYgB1AGcAegBpAGwAbABhAC4AbQBvAHoAaQBsAGwAYQAuAG8AcgBnACAAaAB0AHQAcABzADoALwAvAHQAcgBlAGUAaABlAHIAZABlAHIALgBtAG8AegBpAGwAbABhAC4AbwByAGcALwBhAHAAaQAvAGYAYQBpAGwAdQByAGUAYwBvAHUAbgB0AC8AOwAgAGkAbQBnAC0AcwByAGMAIABoAHQAdABwAHMAOgAvAC8AYgB1AGcAegBpAGwAbABhAC4AbQBvAHoAaQBsAGwAYQAuAG8AcgBnACAAaAB0AHQAcABzADoALwAvAHMAZQBjAHUAcgBlAC4AZwByAGEAdgBhAHQAYQByAC4AYwBvAG0AIABoAHQAdABwAHMAOgAvAC8AdwB3AHcALgBnAG8AbwBnAGwAZQAtAGEAbgBhAGwAeQB0AGkAYwBzAC4AYwBvAG0AOwAgAG8AYgBqAGUAYwB0AC0AcwByAGMAIAAnAG4AbwBuAGUAJwA7ACAAcwBjAHIAaQBwAHQALQBzAHIAYwAgAGgAdAB0AHAAcwA6AC8ALwBiAHUAZwB6AGkAbABsAGEALgBtAG8AegBpAGwAbABhAC4AbwByAGcAIAAnAG4AbwBuAGMAZQAtAG4ARgBPAGMAcQBEAFAAaAAxAEUARQBUAFcANwBOAEwAWQB5ADAAcwBqAHEAcQB3AFgAeABMAHIAbwBCAHAAcQBKAGkAYQBYAGwAaABqADkAQwBQAHEAawBrAEkAYwBmACcAIAAnAHUAbgBzAGEAZgBlAC0AaQBuAGwAaQBuAGUAJwAgAGgAdAB0AHAAcwA6AC8ALwB3AHcAdwAuAGcAbwBvAGcAbABlAC0AYQBuAGEAbAB5AHQAaQBjAHMALgBjAG8AbQA7ACAAcwB0AHkAbABlAC0AcwByAGMAIABoAHQAdABwAHMAOgAvAC8AYgB1AGcAegBpAGwAbABhAC4AbQBvAHoAaQBsAGwAYQAuAG8AcgBnACAAJwB1AG4AcwBhAGYAZQAtAGkAbgBsAGkAbgBlACcAOwAgAGYAcgBhAG0AZQAtAHMAcgBjACAAJwBuAG8AbgBlACcAOwAgAGYAcgBhAG0AZQAtAGEAbgBjAGUAcwB0AG8AcgBzACAAJwBuAG8AbgBlACcAOwAgAGYAbwByAG0ALQBhAGMAdABpAG8AbgAgAGgAdAB0AHAAcwA6AC8ALwBiAHUAZwB6AGkAbABsAGEALgBtAG8AegBpAGwAbABhAC4AbwByAGcAIABoAHQAdABwAHMAOgAvAC8AdwB3AHcALgBnAG8AbwBnAGwAZQAuAGMAbwBtAC8AcwBlAGEAcgBjAGgAIABoAHQAdABwAHMAOgAvAC8AZwBpAHQAaAB1AGIALgBjAG8AbQAvAGwAbwBnAGkAbgAvAG8AYQB1AHQAaAAvAGEAdQB0AGgAbwByAGkAegBlACAAaAB0AHQAcABzADoALwAvAGcAaQB0AGgAdQBiAC4AYwBvAG0ALwBsAG8AZwBpAG4BZT4OTT7kRfqycpfCC8AeuAAAAAAAAAAAwAAAAAAAAEYB3qllfBjPSYS96czvXYq0c5IHOlRteE8wkTq4cYEyCMYAAAAB//////////8BAAAANW1vei1leHRlbnNpb246Ly8yMGI0Yjg2NC00YTkwLTQ3YjQtOTA0ZS1jMDMyYjFhOGI2N2MvAAAAAAAAAA0AAAAQAAAAJAAAABD/////AAAAEP////8AAAAQAAAAJAAAADQAAAABAAAANAAAAAEAAAA0AAAAAQAAADUAAAAAAAAANf////8AAAAA/////wAAADT/////AAAANP////8BAAAAAAABAAAAAAAA' [Exception... "Component returned failure code: 0x80040154 (NS_ERROR_FACTORY_NOT_REGISTERED) [nsISerializationHelper.deserializeObject]" nsresult: "0x80040154 (NS_ERROR_FACTORY_NOT_REGISTERED)" location: "JS frame :: resource://gre/modules/sessionstore/Utils.jsm :: deserializePrincipal :: line 146" data: no] NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsISHEntry.triggeringPrincipal] SessionHistory.jsm:462 If no one else has fixed it tomorrow I will try and fix the test and roll this change into it instead of mine. (I'll remove some of the try checks we have as the Util does it now as pointed out above).
Flags: needinfo?(jkt)
Oh, I guess ExpandedPrincipal had a CID and contract but they were unused. Fix coming up for that.
Attachment #8979751 - Attachment is obsolete: true
> MOZ_ASSERT("We really need to add handling of the old(?) version here") Needs a bool first argument for debug builds. Other than that this fixes the test I just submitted and I could restore manually when checking locally too.
Comment on attachment 8979025 [details] Bug 1452666 - Simplify SessionHistory serialization code and test expanded principals are serialized and can restore tabs. https://reviewboard.mozilla.org/r/245296/#review252204 ::: toolkit/modules/sessionstore/SessionHistory.jsm:213 (Diff revision 7) > } > } > > // Collect triggeringPrincipal data for the current history entry. > if (shEntry.principalToInherit) { > - try { > + entry.principalToInherit_base64 = Utils.serializePrincipal(shEntry.principalToInherit); If shEntry.principalToInherit is null, this will set entry.principalToInherit_base64 to null. Is that the desired behavior? ::: toolkit/modules/sessionstore/SessionHistory.jsm:217 (Diff revision 7) > - debug(e); > - } > } > > if (shEntry.triggeringPrincipal) { > - try { > + entry.triggeringPrincipal_base64 = Utils.serializePrincipal(shEntry.triggeringPrincipal); Again, if triggeringPrincipal is null, will this do the right thing?
Attachment #8979025 - Flags: review?(bzbarsky)
Attachment #8979825 - Attachment is obsolete: true
Comment on attachment 8979025 [details] Bug 1452666 - Simplify SessionHistory serialization code and test expanded principals are serialized and can restore tabs. Also clearing r? for now, pending a different approach...
Attachment #8979025 - Flags: review?(mdeboer)
Comment on attachment 8979025 [details] Bug 1452666 - Simplify SessionHistory serialization code and test expanded principals are serialized and can restore tabs. https://reviewboard.mozilla.org/r/245296/#review252204 > Again, if triggeringPrincipal is null, will this do the right thing? Nither would work here currently however I just wanted to acknoledge this previously was undefined which equally had a problem. Should we attempt to serialize a null principal in these cases?
> this previously was undefined which equally had a problem. What ended up happening in the undefined case? To be clear, I have not been keeping up with the structure and invariants of the session restore code. If the existing code already doesn't handle null (because undefined is not handled in the resulting object), then I think it's fine to keep not handling null with simpler code. I just wanted to raise the question of whether there's a behavior change happening here or not.
> What ended up happening in the undefined case? This was the tab restore issue which this and other related bugs are about. Esentially the tab can't be restored as we are now being more strict about requiring a triggeringPrincipal on deserializing the session restore history item. We could: - Attempt to serialize null pricipal here and use it in the deserialize code - Use null/undefined values as a reason to use a null triggering principal in the deserialize code - Do nothing, the patch you did for serializing of expanded principals should handle all current known serialization issues and perhaps trying to massage any other case for a broken history item is a bad idea anyway. :mikedeboer perhaps you have views on what should happen here too? I think :bz's patch fixes all the known cases and using null here is fine. The previous code used to have an undefined triggeringPrincipal when the Util threw, however a change in the Util already has this code using "null" for these principals. This results in "null" causing a thrown error in deserialize: https://searchfox.org/mozilla-central/rev/eb51d734a8c06ac5973291be9bcc74ed4e999bca/toolkit/modules/sessionstore/SessionHistory.jsm#462 If we leave the attribute to be undefined then we throw an error in DocShell instead: https://searchfox.org/mozilla-central/rev/eb51d734a8c06ac5973291be9bcc74ed4e999bca/docshell/base/nsDocShell.cpp#12279 I suggest we handle "null" before attemting to deserialize in SessionHistory and just ignore serialising, which will result in the docshell code erroring instead.
Flags: needinfo?(mdeboer)
Flags: needinfo?(bzbarsky)
OK. So I _think_ that having a null triggeringPrincipal is not going to happen, so you should just be able to serialize it unconditionally. I don't have strong opinions on what should happen on deserialization if we fail to deserialize a triggering principal... The safest thing to do would be to use a nullprincipal triggering principal. That may fail to do some loads, but will at least try to do something. However having a null principalToInherit is in fact quite possible. Again, how does treatment of "null" differ from "undefined" in the "entry" struct, if it does at all? If it doesn't, then there's no issue here either. We just need to check whether it does.
Flags: needinfo?(bzbarsky)
> However having a null principalToInherit is in fact quite possible. Again, how does treatment of "null" differ from "undefined" in the "entry" struct, if it does at all? Actually I misread the code we shouldn't throw on deserialize for "null" either, however we will assert for triggering principal missing in DocShell. PrincipalToInherit isn't required by DocShell so should be fine. > The safest thing to do would be to use a nullprincipal triggering principal. That may fail to do some loads, but will at least try to do something. :mikedeboer I think the options are: - Continue failing to open tabs without principals in the docshell code - In the deserialize code set a null principal when the base64 or deserialized principal is "null"
>- In the deserialize code set a null principal when the base64 or deserialized principal is "null" That makes sense for triggering principal, but not "principal to inherit". The safe thing for the latter is to use `null`.
(In reply to Jonathan Kingston [:jkt] from comment #39) > :mikedeboer I think the options are: > - Continue failing to open tabs without principals in the docshell code > - In the deserialize code set a null principal when the base64 or > deserialized principal is "null" Okidoke, thanks for narrowing this down to these options. To me the latter option to use a null principal when `null` is returned would be the most reasonable (taking comment 40 into account), since sessionstore is about restoring to a working state as much as possible.
Flags: needinfo?(mdeboer)
So the attached patch, also solves the test case too. I don't touch the principalToInherit as we are set to "undefined"/"null" anyway. The fallback for the triggeringPrincipal allows the extension tab to restore with a null principal. Obviously we should take bz's patch too as there are likely other oddities for not having serialization here, but havind defence in depth for the restoring is pretty great.
Comment on attachment 8979025 [details] Bug 1452666 - Simplify SessionHistory serialization code and test expanded principals are serialized and can restore tabs. https://reviewboard.mozilla.org/r/245296/#review252274 r=me. Thank you for adding the test!
Attachment #8979025 - Flags: review?(bzbarsky) → review+
Comment on attachment 8979025 [details] Bug 1452666 - Simplify SessionHistory serialization code and test expanded principals are serialized and can restore tabs. https://reviewboard.mozilla.org/r/245296/#review252326 LGTM! Thanks, Jonathan! ::: browser/components/extensions/test/browser/browser_ext_sessions_restoreTab.js:12 (Diff revision 8) > + > + > +/** > + This test checks that after closing an extension made tab it restores correctly. > + The tab is given an expanded triggering principal and we didn't use to serialize > + these correctly into session history. nit: indentation hickup. ::: toolkit/modules/sessionstore/SessionHistory.jsm:450 (Diff revision 8) > } > > if (entry.triggeringPrincipal_base64) { > shEntry.triggeringPrincipal = Utils.deserializePrincipal(entry.triggeringPrincipal_base64); > } > + // Ensure that we have a null principal if we couldn't deserialize it nit: missing dots.
Attachment #8979025 - Flags: review?(mdeboer) → review+
Keywords: leave-open
Pushed by jkingston@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/4269e0263226 Simplify SessionHistory serialization code and test expanded principals are serialized and can restore tabs. r=bz,mikedeboer
Comment on attachment 8979945 [details] [diff] [review] Implement nsISerializable on expanded principals Review of attachment 8979945 [details] [diff] [review]: ----------------------------------------------------------------- ::: caps/ExpandedPrincipal.cpp @@ +280,5 @@ > + } > + > + // Play it safe and InsertElementSorted, in case the sort order > + // changed for some bizarre reason. > + mPrincipals.InsertElementSorted(Move(principal), c); Hm. I guess this should be cheap enough that it doesn't matter much... Although, I'm not sure why we're using InsertElementSorted either here or in the constructor. When we're starting with an initially unsorted list, I'd expect Sort() to be way cheaper if the list was long enough for it to matter. ::: caps/ExpandedPrincipal.h @@ +23,5 @@ > static PrincipalKind Kind() { return eExpandedPrincipal; } > > + // For use from the XPCOM factory constructor only. Do not ever use this > + // constructor by hand! > + ExpandedPrincipal(); Maybe make it protected and declare the XPCOM constructor a friend function, then?
Attachment #8979945 - Flags: review?(kmaglione+bmo) → review+
> Maybe make it protected and declare the XPCOM constructor a friend function, then? Tried that, actually. The XPCOM constructor is a static function in nsLayoutModule.cpp. I could make it non-static at the cost of not being able to use the generic factory constructor macro and then friend it, or I can do this...
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #49) > > Maybe make it protected and declare the XPCOM constructor a friend function, then? > > Tried that, actually. The XPCOM constructor is a static function in > nsLayoutModule.cpp. I could make it non-static at the cost of not being > able to use the generic factory constructor macro and then friend it, or I > can do this... Fair enough.
Pushed by bzbarsky@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/de1250f7d42f Implement nsISerializable on expanded principals. r=kmag
Depends on: 1464058
Can we resolve this bug now?
Flags: needinfo?(jkt)
Carrying over the tracking flags from bug 1448309. If the risk isn't crazy-high here, I think we're eventually going to want uplift requests also.
status-firefox62: ? → affected
tracking-firefox60: --- → +
tracking-firefox61: --- → +
tracking-firefox62: --- → +
tracking-firefox-esr60: --- → ?
> Can we resolve this bug now? The only concern is about the regression: Bug 1464058. Personally this seems like a broken test but I won't be able to address it until Tuesday probably. If we get a back out from that then this will be reopened anyway. On the other hand I also think this is safe to uplift once we resolve that.
Flags: needinfo?(jkt)
We don't need to hold this bug open while the Talos regression bug is being investigated still. Jonathan, if you're going to be on PTO for awhile, can you please nominate these patches for uplift now so they're ready for approval once a consensus is reached in bug 1464058? Thanks!
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox62: affected → fixed
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62
Comment on attachment 8979945 [details] [diff] [review] Implement nsISerializable on expanded principals Approval Request Comment [Feature/Bug causing the regression]: Bug 1345433 [User impact if declined]: Some session history won't be serializable and so will cause the tabs to not restore. [Is this code covered by automated tests?]: Yes in the other patch. [Has the fix been verified in Nightly?]: Yes [Needs manual test from QE? If yes, steps to reproduce]: Not required however the STR would verify the patch or running the automated test. [List of other uplifts needed for the feature/fix]: The other patch in this bug: https://bugzilla.mozilla.org/attachment.cgi?id=8979025 [Is the change risky?]: Not really [Why is the change risky/not risky?]: We aren't currently serializing the history for these loads so I can't see what issues this would cause. [String changes made/needed]: N/A
Attachment #8979945 - Flags: approval-mozilla-beta?
Comment on attachment 8979945 [details] [diff] [review] Implement nsISerializable on expanded principals Fix for a couple of session restore and tab close un-do problems. It's baked in Nightly for awhile now without any known issues other than a Talos regression that all signs are pointing to being a flaw in the test. Approved for 61.0b10.
Attachment #8979945 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
This grafts cleanly to ESR60. How do you feel about uplifting there also?
Flags: needinfo?(jkt)
I think we should wait until the next ESR but probably add it there too yeah.
Flags: needinfo?(jkt)
Time for an esr60 uplift request (for 60.2)?
tracking-firefox-esr60: ? → 62+
Flags: needinfo?(jkt)
Missed the uplift sorry, maybe try onto the next one.
Flags: needinfo?(jkt)
There's still a few more weeks in this cycle for ESR 60.2.
Flags: needinfo?(jkt)
Comment on attachment 8979025 [details] Bug 1452666 - Simplify SessionHistory serialization code and test expanded principals are serialized and can restore tabs. [Approval Request Comment] If this is not a sec:{high,crit} bug, please state case for ESR consideration: We risk breaking session history if not uplifted. Both patches in this bug should be uplifted. User impact if declined: Some session history won't be serializable and so will cause the tabs to not restore. Fix Landed on Version: 62 Risk to taking this patch (and alternatives if risky): We aren't currently serializing the history for these loads so I can't see what issues this would cause. String or UUID changes made by this patch: N/A See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.
Flags: needinfo?(jkt)
Attachment #8979025 - Flags: approval-mozilla-esr60?
Comment on attachment 8979025 [details] Bug 1452666 - Simplify SessionHistory serialization code and test expanded principals are serialized and can restore tabs. Fixes issues with broken session restore. Approved for ESR 60.2.
Attachment #8979025 - Flags: approval-mozilla-esr60? → approval-mozilla-esr60+
Flags: qe-verify+
I managed to reproduce the bug on Windows 10, Nightly 60.0a1 (2018-03-01). I verified it on Firefox 61.0.2 and Firefox 62.0b15 and it is not reproducing. On Firefox 60.1.0esr is still reproducible. I will mark 61 and 62 as Verified.
status-firefox61: fixed → verified
status-firefox62: fixed → verified
60.1.0esr was released in June so can't have a patch from yesterday. You can grab current builds off of treeherder and verify there.
Flags: needinfo?(david.olah)
Thanks Julien. I verified it on Firefox 60.1.1esr and it is not reproducing anymore. Will also mark esr62 as Verified. Just to be sure I also verified on the Latest Nightly and the issue is not reproducible.
Status: RESOLVED → VERIFIED
status-firefox-esr60: fixed → verified
Flags: qe-verify+
Flags: needinfo?(david.olah)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: