User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.13; rv:63.0) Gecko/20100101 Firefox/63.0 Build ID: 20180816220128 Steps to reproduce: Filing a new bug as per :standard8's request in bug 1483148 comment 5. Bookmarklets (bookmarks with a javascript: URI) no longer work after disallowInheritPrincipal bug 1466801. Executing those same bookmarklets with a keyword still works. Executing those same bookmarklets with a keyword but in a new tab with Alt+Enter no longer works. 1) Create a bookmarklet with URI: javascript:alert(%s) 2) Give it a keyword: xxx 3) Save 4) In the location bar, enter xxx 1 followed by Enter 5) In the location bar, enter xxx 2 followed by Alt+Enter Actual results: 4) Pops up an alert box with “1” 5) Opens a new tab, but does not show the alert

jkt, can you pick this up as well?

Yeah will take a look next.

[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.

Comment on attachment 9006191 [details] Bug 1484741 - Fixing bookmarklet keywords from loading in a new tab. :Gijs (he/him) has approved the revision.

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 jkingston@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/7d8d3fc3e9f4 Fixing bookmarklet keywords from loading in a new tab. r=Gijs