46 bytes, text/x-phabricator-request
|Details | Review|
jkt, can you pick this up as well?
Yeah will take a look next.
Assignee: nobody → jkt
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Priority: -- → P1
[Tracking Requested - why for this release]: What's blocking the fix here? This is riding with 63 atm, but given it's a regression I don't think we should be shipping this.
Jonathan, do you have any update? Thanks
No update currently. I was looking at this last week and got confused by some edge cases that I still need to look into.
So I'm seeing that the contentPrincipal of the about:blank page is a null principal. It looks like we can't create about:blank principals (we explicitly test for this in: caps/tests/unit/test_origin.js). We call addTab and loadUri into it, loadURIWithOptions In nsJSProtocolHandler.cpp we check the principal subsumes the object principal before executing and return NS_ERROR_DOM_RETVAL_UNDEFINED because nothing but a system principal executes into null here other than the exact same. (https://searchfox.org/mozilla-central/rev/a23c3959b62cd3f616435e02810988ef5bac9031/dom/jsurl/nsJSProtocolHandler.cpp#249-255) The submitted solution instead forces allowInheritPrincipal which copies the created null principal of the about:blank page as we can't inherit the system principal (both docshell and the protocol code are preventing this). I'm leaning on just ignoring the location this should be created into given I don't understand the use-case of doing this really. > I'm a little confused why this fixes anything. I also think we need to ensure we actually have a principal here, which AFAICT we don't in the normal return case, so presumably then this would inherit system principal, which seems bad. Can you elaborate on how this fix works? Checking and preventing system in the urlbarBindings to allowInheritPrincipal actually breaks the code as docShell does the right thing for us when the about:blank is created. I don't think I can pass anything into docshell here that is the correct triggeringPrincipal as my understanding is that docshell is creating the new principal within the load. I still need to diagnose further how we are passing the correct null principal even when system is passed though to see if we can do something better here.
Any progress on the further diagnosing?
The code that make this safe is after: > One more twist: Don't inherit the principal for external loads. https://searchfox.org/mozilla-central/rev/ce57be88b8aa2ad03ace1b9684cd6c361be5109f/docshell/base/nsDocShell.cpp#9361 :Gijs Unless you have another direction of fixing this, I don't see any alternative to the already submitted patch (minus something more drastic into the docshell code). Essentially the problem is we create a new tab with a null principal and anything passed in doesn't match that. I think in the long run we should instead pass in a null triggeringPrincipal and create the tab with that.
Flags: needinfo?(jkt) → needinfo?(gijskruitbosch+bugs)
Comment on attachment 9006191 [details] Bug 1484741 - Fixing bookmarklet keywords from loading in a new tab. :Gijs (he/him) has approved the revision.
Attachment #9006191 - Flags: review+
Jonathan, do you want to land this patch and then request an uplift to 63?
I personally think we shouldn't uplift the patch I would rather we test it more in Nightly first. Despite the code working I'm not 100% on if we are doing the right thing still. Sorry for the delay here.
(In reply to Jonathan Kingston [:jkt] from comment #12) > I personally think we shouldn't uplift the patch I would rather we test it > more in Nightly first. Despite the code working I'm not 100% on if we are > doing the right thing still. > > Sorry for the delay here. This hasn't actually landed on nightly, AFAICT... Did you intend for it to land on 64 nightly? :-)
I triggered it on lando with that comment, it's queued to go in now.
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/autoland/rev/7d8d3fc3e9f4 Fixing bookmarklet keywords from loading in a new tab. r=Gijs
You need to log in before you can comment on or make changes to this bug.