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)
Core
DOM: Security
Tracking
()
RESOLVED
FIXED
mozilla59
Tracking | Status | |
---|---|---|
firefox59 | --- | fixed |
People
(Reporter: ckerschb, Assigned: tnguyen)
References
Details
(Whiteboard: [domsecurity-active])
Attachments
(3 files)
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.
Reporter | ||
Updated•8 years ago
|
Reporter | ||
Comment 1•8 years ago
|
||
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.
![]() |
||
Updated•8 years ago
|
Flags: needinfo?(jwatt)
I revert Bug 1334875, https://gist.github.com/allstarschh/e7830dc55dd6e08c52d4e66c4155ff42
However I am not able to reproduce the assert, including the STR from https://bugzilla.mozilla.org/show_bug.cgi?id=1334875#c25, https://bugzilla.mozilla.org/show_bug.cgi?id=1334875#c34 and https://bugzilla.mozilla.org/show_bug.cgi?id=1334875#c35
Assignee | ||
Comment 3•7 years ago
|
||
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.
Assignee | ||
Comment 4•7 years ago
|
||
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
Updated•7 years ago
|
Assignee: nobody → tnguyen
Assignee | ||
Comment 5•7 years ago
|
||
Well it failed in try test so I could take a look
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d4c6275ffbffd0c73596e6562ab9b975e279162d&selectedJob=137454790
Assignee | ||
Comment 6•7 years ago
|
||
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 9•7 years ago
|
||
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?
Reporter | ||
Comment 10•7 years ago
|
||
mozreview-review |
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)
Reporter | ||
Comment 11•7 years ago
|
||
mozreview-review |
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+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 14•7 years ago
|
||
(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 15•7 years ago
|
||
mozreview-review |
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+
Assignee | ||
Comment 16•7 years ago
|
||
(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 :)
Assignee | ||
Comment 17•7 years ago
|
||
Comment 18•7 years ago
|
||
hg error in cmd: hg rebase -s e6fd83e8a89e -d a74a0791fbb8: abort: can't rebase public changeset e6fd83e8a89e
(see 'hg help phases' for details)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 21•7 years ago
|
||
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
Comment 22•7 years ago
|
||
Backed out in https://hg.mozilla.org/integration/autoland/rev/083251780a86 for eslint bustage, https://treeherder.mozilla.org/logviewer.html#?job_id=141222931&repo=autoland
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 25•7 years ago
|
||
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)
Comment 26•7 years ago
|
||
Also Android testSessionOOMRestore bustage: the primary form was https://treeherder.mozilla.org/logviewer.html#?job_id=141228521&repo=autoland but there's a nice variety of fails in https://treeherder.mozilla.org/#/jobs?repo=autoland&fromchange=7353f6f685716931addf286fb5d475a5464ab0e0&filter-searchStr=f1410fdb3242c4872b020f813a8bdd0af1fcd7b1&tochange=da60fbc995312439ae99a50f28816da360da787b
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8921289 -
Flags: review?(cnevinchen)
Comment 29•7 years ago
|
||
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 30•7 years ago
|
||
mozreview-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 31•7 years ago
|
||
mozreview-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+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8921289 -
Flags: review?(cnevinchen)
Assignee | ||
Comment 34•7 years ago
|
||
That's a good point, how about putting system principal in the basic session store data of zombie tab
Comment 35•7 years ago
|
||
(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 36•7 years ago
|
||
mozreview-review |
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+
Assignee | ||
Comment 37•7 years ago
|
||
(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)
Assignee | ||
Comment 38•7 years ago
|
||
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.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Reporter | ||
Comment 41•7 years ago
|
||
(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).
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 44•7 years ago
|
||
rebased and running try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fb1d0d893c7db72fba798b55ec19ce378a827bb1
Comment 45•7 years ago
|
||
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
Comment 46•7 years ago
|
||
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.
Assignee | ||
Comment 47•7 years ago
|
||
(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]
Comment 48•7 years ago
|
||
(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! :)
Assignee | ||
Comment 49•7 years ago
|
||
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
Comment 50•7 years ago
|
||
bugherder |
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
![]() |
||
Updated•7 years ago
|
Flags: needinfo?(jwatt)
You need to log in
before you can comment on or make changes to this bug.
Description
•