Bug 1529901 Comment 13 Edit History

Note: The actual edited comment in the bug view page will always show the original commenter’s name and original timestamp.

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #10)

> Gijs, I talked things over with Jonathan, here are our findings.
> 
> (In reply to :Gijs (he/him) from comment #9)
> > 1) we used to always use system principal for saves. bug 1398229 made us use the node's principal. That seems to have just not worked for the http auth case. I don't really know why. I don't even know if the preflight request ever did anything better when it did use system principal (ie maybe it didn't work before we started passing the node's principal). Anyway, now we're passing loading principal. I find this a little odd; is loading principal really the best idea if we have webpage 1 (html) linking to webpage 2 (also html) ? Shouldn't it be triggering principal? It's just a link, not an included image or other included resource.
> 
> Right, so good we are not using the SystemPrincipal anymore, that was definitely wrong. Jonathan and I are with you, we should in fact use the triggeringPrincipal (which we suppose is this.principal) instead of the node's principal.

No, my point is that we're passing a principal to channel creation, but we pass it as the loadingPrincipal, and I think we should be passing as the triggering principal. It'd be the same principal. I wonder if that's why we're not getting a dialog and so on. Though I haven't tested it, and I'm not sure I fully understand what the consequence of passing triggering instead of loading principal would be. Ni for this.

> > 3) then I made webbrowserpersist / saving anything *require* a principal, in bug 1469916. But this code didn't pass one, so it crashed, until I fixed bug 1473507. I thought I got all the consumers there (ie added principals everywhere), clearly I was wrong. I blame the insane amount of indirection we have for saving anything - there must be at least 10 different utility functions that eventually call internalSave, and they get called all over the shop.
>
> So, that quirks we have accumulated over time within internalSave() to query the right principal [1] makes me nervous quite a bit. We should file a follow up bug, adding some kind of assertion (similar to what we did for providing the top-level triggeringPrincipal) that we *always* pass an explicit TriggeringPrincipal to internalSave().

> 
> [1] https://searchfox.org/mozilla-central/rev/92d11a33250a8e368c8ca3e962e15ca67117f765/toolkit/content/contentAreaUtils.js#432-435
> 
> > Anyway... the trivial fix is to just add a principal to the fallback case. I've added a patch for that. But I'm confused about why the preflight request doesn't handle the http auth prompt itself, ie why are we hitting the fallback? Christoph/Baku, can you check both why we're not handling http auth for the initial request and whether loading principal is really right here, or should the initial request use triggering principal, too?
> 
> The patch you have looks good, but I can't help you with the http auth problem - I have no idea why we are hitting the fallback. Probably Dragana or someone from the Necko team can be of more help with that problem.
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #10)

> Gijs, I talked things over with Jonathan, here are our findings.
> 
> (In reply to :Gijs (he/him) from comment #9)
> > 1) we used to always use system principal for saves. bug 1398229 made us use the node's principal. That seems to have just not worked for the http auth case. I don't really know why. I don't even know if the preflight request ever did anything better when it did use system principal (ie maybe it didn't work before we started passing the node's principal). Anyway, now we're passing loading principal. I find this a little odd; is loading principal really the best idea if we have webpage 1 (html) linking to webpage 2 (also html) ? Shouldn't it be triggering principal? It's just a link, not an included image or other included resource.
> 
> Right, so good we are not using the SystemPrincipal anymore, that was definitely wrong. Jonathan and I are with you, we should in fact use the triggeringPrincipal (which we suppose is this.principal) instead of the node's principal.

No, my point is that we're passing a principal to channel creation, but we pass it as the loadingPrincipal, and I think we should be passing as the triggering principal. It'd be the same principal. I wonder if that's why we're not getting a dialog and so on. Though I haven't tested it, and I'm not sure I fully understand what the consequence of passing triggering instead of loading principal would be. Ni for this.

Back to Bug 1529901 Comment 13