Closed Bug 1345433 Opened 8 years ago Closed 7 years ago

Bring back assertion that history entries need a valid triggeringPrincipal

Categories

(Core :: DOM: Security, enhancement, P2)

Product:
Core
Component:
DOM: Security
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
firefox59 --- fixed

People

(Reporter: ckerschb, Assigned: tnguyen)

References

Details

(Whiteboard: [domsecurity-active])

Attachments

(3 files)

sessionstoreFailed.zip
7.92 KB, application/zip
Details
Bug 1345433 - Bring back assertion that history entries need a valid triggeringPrincipal
59 bytes, text/x-review-board-request
smaug
: review+
JanH
: review+
Details
Bug 1345433 - Ensure tests load pass valid triggeringPrincipal.
59 bytes, text/x-review-board-request
ckerschb
: review+
Details
Within Bug 1334875 we removed the assertion that history entries need a valid triggeringPricnipal. Once we have time we should try to get the STRs provided within Bug 1334875, debug that and bring back the assertion.
Depends on: 1334875, 1307736
Priority: -- → P3
Whiteboard: [domsecurity-backlog1]
In fact within ::LoadHistoryEntry() we should also remove the fallback + if (!triggeringPrincipal) { + triggeringPrincipal = nsContentUtils::GetSystemPrincipal(); + } At the same time we bring back the assertion. We also need to make sure that all the STRs provided in Bug 1334875 to trigger the assertion get tested and resolved before relanding that assertion.
Flags: needinfo?(jwatt)
I took a look at the STRs in bug 1334875, and likely people see the assession after re-starting from killing ff process, or encountering previous crashes. The interesting point is that, in https://bugzilla.mozilla.org/show_bug.cgi?id=1334875#c25 I could see triggerringPrinciple was missing from recovery.js I still have no idea why we have ended with that. I guess there's certain corner case when we save the recovery.jsonjz4 ( probably due to crashes, or shutting down manually), and we did not write serialized triggerringPrincipal correctly.
Attached file sessionstoreFailed.zip —
I tried to remove only triggeringPrincipal_base64 attr from the jsonjz4 file and I could easily reproduce the crash with the latest m-c debug build. I think we should not bring the assertion back until we find out why we save recovery jsonjz4 wrongly
Assignee: nobody → tnguyen
I don't see the issue happens again. One case I was thinking about is people restore sessions which were serialized in a different format (for example triggeringPrincipalBase64 instead of triggeringPrincipal_base64). But it's a long time ago and we may not have to worry about that. Brought the assertion back and updated the tests, Christoph, could you please take a look?
Comment on attachment 8921288 [details] Bug 1345433 - Bring back assertion that history entries need a valid triggeringPrincipal https://reviewboard.mozilla.org/r/192308/#review198150 We initially added the assertion within Firefox54 (see Bug 1307736), that was quite so many releases ago. I think we can bring the assertion back now. Once you have updated the patch please file smaug or bz for a final check if they are fine with bringing back the assertion as well. ::: docshell/base/nsDocShell.cpp (Diff revision 1) > srcdoc = VoidString(); > } > > - if (!triggeringPrincipal) { > - triggeringPrincipal = nsContentUtils::GetSystemPrincipal(); > + // If there is no valid triggeringPrincipal, we deny the load > + MOZ_ASSERT(triggeringPrincipal, "need a valid triggeringPrincipal to load from history"); > - } Only asserting is not enough, because the MOZ_ASSERT macro turns into a no-op for release version. You also need. if (!triggeringPrincipal) { return NS_ERROR_FAILURE; } ::: docshell/shistory/nsSHEntry.cpp:553 (Diff revision 1) > } > > NS_IMETHODIMP > nsSHEntry::SetTriggeringPrincipal(nsIPrincipal* aTriggeringPrincipal) > { > + MOZ_ASSERT(aTriggeringPrincipal, "need a valid triggeringPrincipal"); same, here. if (!aTriggeringPricnipal) { return error; }
Attachment #8921288 - Flags: review?(ckerschb)
Comment on attachment 8921289 [details] Bug 1345433 - Ensure tests load pass valid triggeringPrincipal. https://reviewboard.mozilla.org/r/192310/#review198154 Test updates look reasonable to me. thanks.
Attachment #8921289 - Flags: review?(ckerschb) → review+
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #10) > Comment on attachment 8921288 [details] > Bug 1345433 - Bring back assertion that history entries need a valid > triggeringPrincipal > > https://reviewboard.mozilla.org/r/192308/#review198150 > > We initially added the assertion within Firefox54 (see Bug 1307736), that > was quite so many releases ago. I think we can bring the assertion back now. > Once you have updated the patch please file smaug or bz for a final check if > they are fine with bringing back the assertion as well. Yes, that's true, thanks Christoph. Smaug, do you agree to bring it back?
Comment on attachment 8921288 [details] Bug 1345433 - Bring back assertion that history entries need a valid triggeringPrincipal https://reviewboard.mozilla.org/r/192308/#review198480 I guess this is fine for now, but MOZ_ASSERT(foo); if (!foo) { } is basically an anti-pattern. Why do we have the 'if', if we just asserted that it can never happen. At some point we should get rid of at least the assertion in LoadHistoryEntry. ::: docshell/shistory/nsSHEntry.cpp:553 (Diff revision 2) > } > > NS_IMETHODIMP > nsSHEntry::SetTriggeringPrincipal(nsIPrincipal* aTriggeringPrincipal) > { > + MOZ_ASSERT(aTriggeringPrincipal, "need a valid triggeringPrincipal"); In principle this is wrong, since one can call this API with null value from JS just fine. But I guess this may help ensuring one isn't accidentally passing null in Firefox UI code.
Attachment #8921288 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] from comment #15) > Comment on attachment 8921288 [details] > Bug 1345433 - Bring back assertion that history entries need a valid > triggeringPrincipal > > https://reviewboard.mozilla.org/r/192308/#review198480 > > I guess this is fine for now, but > MOZ_ASSERT(foo); > if (!foo) { > } > > is basically an anti-pattern. Why do we have the 'if', if we just asserted > that it can never happen. > At some point we should get rid of at least the assertion in > LoadHistoryEntry. You are right, that's my original patch. We don't expect a null triggeringPrincipal here, but I guess we may not want to break release build in a corner case we've never seen. Or we may encounter null pointer crash then fix that. > ::: docshell/shistory/nsSHEntry.cpp:553 > (Diff revision 2) > > } > > > > NS_IMETHODIMP > > nsSHEntry::SetTriggeringPrincipal(nsIPrincipal* aTriggeringPrincipal) > > { > > + MOZ_ASSERT(aTriggeringPrincipal, "need a valid triggeringPrincipal"); > > In principle this is wrong, since one can call this API with null value from > JS just fine. > But I guess this may help ensuring one isn't accidentally passing null in > Firefox UI code. Right, we don't want passing null :)
hg error in cmd: hg rebase -s e6fd83e8a89e -d a74a0791fbb8: abort: can't rebase public changeset e6fd83e8a89e (see 'hg help phases' for details)
Pushed by tnguyen@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/6ee966b99e7e Bring back assertion that history entries need a valid triggeringPrincipal r=smaug https://hg.mozilla.org/integration/autoland/rev/7353f6f68571 Ensure tests load pass valid triggeringPrincipal. r=ckerschb
As a ignore warning like bug 1240167 to remove no-unused-var warning in head.js (because actually the variable will be used in another file)
Attachment #8921289 - Flags: review?(cnevinchen)
Comment on attachment 8921289 [details] Bug 1345433 - Ensure tests load pass valid triggeringPrincipal. Thanks Thomas. It looks good to me. But maybe Jan knows better.
Attachment #8921289 - Flags: review?(jh+bugzilla)
Attachment #8921289 - Flags: review?(cnevinchen)
Attachment #8921289 - Flags: review+
Comment on attachment 8921288 [details] Bug 1345433 - Bring back assertion that history entries need a valid triggeringPrincipal https://reviewboard.mozilla.org/r/192308/#review206746 On Android we add some basic session store data as soon as a tab is created (and before we've actually started loading the URL) [1], because e.g. a new tab opened in background could, if unlucky, get zombified (discarded) before it actually had a chance to load far enough for the session store to collect its data normally. Since we rely on the session store data to revive a discarded tab, we'd be stuck otherwise (bug 1229259). So you need to add some sort of principal there that tides us over until the tab has loaded far enough to trigger a regualar session store data collection. [1] https://dxr.mozilla.org/mozilla-central/rev/b056526be38e96b3e381b7e90cd8254ad1d96d9d/mobile/android/chrome/content/browser.js#3696
Attachment #8921288 - Flags: review-
Comment on attachment 8921289 [details] Bug 1345433 - Ensure tests load pass valid triggeringPrincipal. https://reviewboard.mozilla.org/r/192310/#review206778 ::: mobile/android/tests/browser/robocop/src/org/mozilla/gecko/tests/SessionTest.java:380 (Diff revision 5) > asserter.is(url, page.url, "URL in JSON matches session URL"); > if (!page.url.startsWith("about:")) { > asserter.is(title, page.title, "title in JSON matches session title"); > } > + > + asserter.is(principal, page.triggeringPrincipal_base64, If I'm reading this right, for testSessionOOMSave you're implicitly relying on the fact that opening a new tab in the UI uses the same principal you've hard-coded in the page info above, but as long as you're aware of this and this fact (opening a new tab using that principal) is reasonably stable I guess it is fine.
Attachment #8921289 - Flags: review?(jh+bugzilla) → review+
Attachment #8921289 - Flags: review?(cnevinchen)
That's a good point, how about putting system principal in the basic session store data of zombie tab
(In reply to Thomas Nguyen[:tnguyen] ni? plz from comment #34) > That's a good point, how about putting system principal in the basic session > store data of zombie tab Your call - I'm not really familiar with principals, so I can just point out that with this assertion in place we need to do something here as well. Do new tabs always start out with the system principal, or could they get some other triggering principal in certain cases? And if yes, would it be a problem if tabs that are either opened as zombified background tabs right from the start [1], or else zombified before they had a chance to fully load end up with the system principal after they're eventually restored, instead of the triggering principal that would have ordinarily been used? [1] Tabs opened by the session store don't count, since in that case we reattach the full previous session store data right away.
Comment on attachment 8921288 [details] Bug 1345433 - Bring back assertion that history entries need a valid triggeringPrincipal https://reviewboard.mozilla.org/r/192308/#review206858 Fine from a mobile session store perspective, though I've got no idea whether this could have unwanted side effects for the DOM Security side.
Attachment #8921288 - Flags: review+
(In reply to Jan Henning [:JanH] from comment #35) > (In reply to Thomas Nguyen[:tnguyen] ni? plz from comment #34) > Do new tabs always start out with the system principal, or could they get > some other triggering principal in certain cases? And if yes, would it be a > problem if tabs that are either opened as zombified background tabs right > from the start [1], or else zombified before they had a chance to fully load > end up with the system principal after they're eventually restored, instead > of the triggering principal that would have ordinarily been used? > > > [1] Tabs opened by the session store don't count, since in that case we > reattach the full previous session store data right away. I think you are right. As I understand correctly, the case is only in mobile, and entries that need to be initialized for delay loading. But I have no idea how to tie the principal here, particularly in some cases, we add new tab with null principal in params (we called addTab in many places without putting principal, but I guess we will eventually add the triggering principal before pass it to docshell and channel)
At the moment (before we land this fix), we will fallback to system principal if null, so I think it would be ok if we put system principal here. This is only for the cases, in android, we do a zombified tab right after creation or before fully loading tab.
(In reply to Thomas Nguyen[:tnguyen] ni? plz from comment #38) > At the moment (before we land this fix), we will fallback to system > principal if null, so I think it would be ok if we put system principal > here. This is only for the cases, in android, we do a zombified tab right > after creation or before fully loading tab. I think I am fine with this. It's not any worth than it is right now, right? What I suggest is, file a follow up for mobile and add a comment in the code (maybe even including the bug number).
Pushed by tnguyen@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/1294e490afc2 Bring back assertion that history entries need a valid triggeringPrincipal r=JanH,smaug https://hg.mozilla.org/integration/autoland/rev/166fb0b61359 Ensure tests load pass valid triggeringPrincipal. r=ckerschb,JanH
this seems to have caused a large perf win for session restore: == Change summary for alert #10834 (as of Fri, 01 Dec 2017 12:17:16 GMT) == Improvements: 63% sessionrestore_many_windows windows10-64 pgo e10s 3,500.42 -> 1,297.92 For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=10834 I also see improvements on other OS's, just don't have the alerts at the moment.
(In reply to Joel Maher ( :jmaher) (UTC-5) from comment #46) > this seems to have caused a large perf win for session restore: > == Change summary for alert #10834 (as of Fri, 01 Dec 2017 12:17:16 GMT) == > > Improvements: > > 63% sessionrestore_many_windows windows10-64 pgo e10s 3,500.42 -> > 1,297.92 > > For up to date results, see: > https://treeherder.mozilla.org/perf.html#/alerts?id=10834 > > > I also see improvements on other OS's, just don't have the alerts at the > moment. oh, great, thanks for the notice
Priority: P3 → P2
Whiteboard: [domsecurity-backlog1] → [domsecurity-active]
(In reply to Joel Maher ( :jmaher) (UTC-5) from comment #46) > this seems to have caused a large perf win for session restore: > ... > I also see improvements on other OS's, just don't have the alerts at the > moment. Yeah, this is great news! Thanks for the info, Joel! :)
I thought a bit more. Obviously, we got the perf win alert means, the patch changed session store's code flow (e.g bailed out). Likely we still push empty triggeringPrincial somewhere, and I will not be surprised if there's any new bug filed for that (then it will be what we have to fix)
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/1294e490afc2 https://hg.mozilla.org/mozilla-central/rev/166fb0b61359
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Flags: needinfo?(jwatt)
Depends on: 1448309
Depends on: 1449301
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: