Closed
Bug 1332595
Opened 7 years ago
Closed 7 years ago
about:newtab cannot load about:* tiles
Categories
(Firefox :: General, defect, P1)
Tracking
()
VERIFIED
FIXED
Firefox 54
Tracking | Status | |
---|---|---|
firefox50 | --- | unaffected |
firefox51 | --- | unaffected |
firefox52 | --- | unaffected |
firefox53 | + | verified |
firefox54 | --- | verified |
People
(Reporter: Fanolian+BMO, Assigned: Gijs)
References
Details
(Keywords: regression, reproducible, Whiteboard: [domsecurity-active])
Attachments
(1 file, 2 obsolete files)
59 bytes,
text/x-review-board-request
|
mossop
:
review+
lizzard
:
approval-mozilla-aurora+
|
Details |
## Carrying forward from bug 1329457 comment 22 and cloning from bug 1329032 & bug 1331686. Steps to reproduce: 1. Bookmark some about:* pages, e.g. about:crashes. 2. Open about:newtab. 3. Drag about:crashes bookmark into about:newtab to make a new tile. 4. Click on the tile. Actual result: Error shown in Browser Console: > Security Error: Content at about:newtab may not load or link to about:crashes. a) Opening about:newtab and clicking on about:crashes DOES NOT WORK b) Opening about:newtab and CTRL+CLICK on about:crashes does not work c) Opening about:newtab and right-click->open in new tab does not work d) Opening about:newtab and right-click->open in new window does not work e) Opening about:newtab and drag/drop about:crashes into a new tab WORKS
Bug 1329032 did not fix the single left-click with no modifiers issue. (case a) I didn't test the patch in bug 1331686 yet because I don't know how. :( CCing :ckerschb because he is working on relevant bugs.
[Tracking Requested - why for this release]: Regression caused by Bug 1182569
tracking-firefox53:
--- → ?
Assignee | ||
Comment 3•7 years ago
|
||
This is because https://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#3068 explicitly tries to load the page through openLinkIn, which we're only getting around to fixing in bug 1331686 (I think?). Mike, the comment above that code suggests the only reason this code exists is because about:newtab used to load in the parent and the pages want to load in the child. I think that's not true anymore, plus I think we now have the browser remoteness swapping infrastructure... so maybe this code can just be ripped out, which would simplify things and not require the fix for bug 1331686 before this bug would be fixed?
Flags: needinfo?(mconley)
Updated•7 years ago
|
Status: UNCONFIRMED → NEW
status-firefox50:
--- → unaffected
status-firefox51:
--- → unaffected
status-firefox52:
--- → unaffected
status-firefox53:
--- → affected
Ever confirmed: true
Version 54.0a1 Build ID 20170123080351 Bug 1331686 does not fix this issue in 2017-01-23 build. I found a workaround that makes about:* tiles work in about:newtab. That is to manually load an about:* URL in the about:new tab URL bar once. (or any about:* URL bar) Workaround STR: 1. Make sure an about:* tile is not working in about:newtab. 2. Go to URL bar and type about:crashes. Press Enter. 3. Open a new about:newtab. Click on a about:* tile. It should work now until exiting browser. This workaround may have existed before fixing bug 1331686 and bug 1329032. Alternative workaround: 1. Make sure an about:* tile is not working in about:newtab. 2. Open about:support in current tab or a new tab. Click on the link to about:plugins. 3. Open a new about:newtab. Click on a about:* tile. It should work now until exiting browser.
Assignee | ||
Comment 5•7 years ago
|
||
comment #4 is scary because it suggests we're using the wrong principal entirely somewhere. :-\
Flags: needinfo?(ckerschb)
Comment 6•7 years ago
|
||
I can reproduce the problem described within comment 0. It seems like a very related problem where we are not passing the right principal (at least from a first inspection) and hence fall back to creating a principal from the referrer: doContentSecurityCheck { channelURI: about:crashes loadingPrincipal: nullptr triggeringPrincipal: about:newtab principalToInherit: nullptr contentPolicyType: 6 securityMode: SEC_ALLOW_CROSS_ORIGIN_DATA_IS_NULL, initalSecurityChecksDone: no enforceSecurity: no } I will take a closer look (but probably not today).
Assignee: nobody → ckerschb
Status: NEW → ASSIGNED
Flags: needinfo?(ckerschb)
Priority: -- → P1
Whiteboard: [domsecurity-active]
Comment 7•7 years ago
|
||
Gijs, I think ultimately we want to have Bug 1333030 fixed. As far as this bug goes, we were only missing to pass the originPrincipal when calling openLinkIn() within borwser.js The new arguments for the call are: doContentSecurityCheck { channelURI: about:crashes loadingPrincipal: nullptr triggeringPrincipal: SystemPrincipal principalToInherit: nullptr contentPolicyType: 6 securityMode: SEC_ALLOW_CROSS_ORIGIN_DATA_IS_NULL, initalSecurityChecksDone: no enforceSecurity: no } I am not entirely sure, but do you think we also need to pass an OriginPrincipal within _loadSearch within browser.js? That's the other callsite that does not explicitly pass an originPrincipal within browser.js.
Attachment #8829893 -
Flags: review?(gijskruitbosch+bugs)
Assignee | ||
Updated•7 years ago
|
Attachment #8829893 -
Flags: review?(gijskruitbosch+bugs) → review+
Updated•7 years ago
|
Keywords: checkin-needed
Comment 8•7 years ago
|
||
Comment on attachment 8829893 [details] [diff] [review] bug_1332595_fix_missing_triggeringprincipal_for_tiles.patch Approval Request Comment [Feature/Bug causing the regression]: Bug 1182569 - Use asyncOpen2() for docshell loads (nsJSProtocolHandler and nsURILoader) [User impact if declined]: As described in comment 0. Users adding e.g. about:crashes as a new tile within about:newtab can not load about:crashes when clicking on the tile. [Is this code covered by automated tests?]: No, automated test, only manual test (see STR from comment 0). [Has the fix been verified in Nightly?]: Not yet - will have to follow up on that. [Needs manual test from QE? If yes, steps to reproduce]: No need, but STR from comment 0. [List of other uplifts needed for the feature/fix]: no uplfits needed. [Is the change risky?]: No. [Why is the change risky/not risky?]: We are only passing a triggeringPrincipal argument which was previously left empty. [String changes made/needed]: No
Attachment #8829893 -
Flags: approval-mozilla-aurora?
Comment 9•7 years ago
|
||
(In reply to :Gijs from comment #3) > Mike, the comment above that code suggests the only reason this code exists > is because about:newtab used to load in the parent and the pages want to > load in the child. I think that's not true anymore, Well, about:newtab is (unfortunately) still loaded in the parent process. > plus I think we now have > the browser remoteness swapping infrastructure... so maybe this code can > just be ripped out, which would simplify things and not require the fix for > bug 1331686 before this bug would be fixed? That's true - the remoteness flipping stuff _should_ do the right thing here. I suspect this event handler can just go away (and then the BrowserOnClick can lose handleEvent entirely). I haven't tested this, but I'm 90% confident.
Flags: needinfo?(mconley)
Comment 10•7 years ago
|
||
I'm now 100% confident. I locally removed the click event listener and about:newtab continues to behave properly (and we do the remoteness flip as expected). We should probably just go that route instead.
Assignee | ||
Comment 11•7 years ago
|
||
(In reply to Mike Conley (:mconley) from comment #10) > I'm now 100% confident. I locally removed the click event listener and > about:newtab continues to behave properly (and we do the remoteness flip as > expected). > > We should probably just go that route instead. I concur. Christoph, do you want to write this patch or shall I?
Flags: needinfo?(ckerschb)
Comment 12•7 years ago
|
||
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/08db703246b2 Fix missing triggeringPrincipal within browser.js. r=gijs
Keywords: checkin-needed
Comment 13•7 years ago
|
||
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/259be2f691b5 for (apparently e10s-only, but both Mac and Windows) https://treeherder.mozilla.org/logviewer.html#?job_id=71778201&repo=mozilla-inbound
Comment 14•7 years ago
|
||
(In reply to :Gijs from comment #11) > > We should probably just go that route instead. > > I concur. Christoph, do you want to write this patch or shall I? Gijs, I would appreciate if you could take over in that case. Just to be clear, that means there is no need to land the attached patch, right? In either case, any idea why we would face the problem with gSerializationHelper.serializePrincipal [see detailed error message underneath and also comment 13]? Is there something we need to udpate or can/should we ignore that error message? > [JavaScript Error: "gSerializationHelper.serializePrincipal is not a function" {file: "chrome://browser/content/browser.js" line: 908}
Flags: needinfo?(ckerschb) → needinfo?(gijskruitbosch+bugs)
Assignee | ||
Comment 15•7 years ago
|
||
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #14) > (In reply to :Gijs from comment #11) > > > We should probably just go that route instead. > > > > I concur. Christoph, do you want to write this patch or shall I? > > Gijs, I would appreciate if you could take over in that case. Just to be > clear, that means there is no need to land the attached patch, right? OK. And yes. > In > either case, any idea why we would face the problem with > gSerializationHelper.serializePrincipal [see detailed error message > underneath and also comment 13]? Is there something we need to udpate or > can/should we ignore that error message? > > > [JavaScript Error: "gSerializationHelper.serializePrincipal is not a function" {file: "chrome://browser/content/browser.js" line: 908} The method on the serializationhelper is called serializeToString, not serializePrincipal.
Assignee: ckerschb → gijskruitbosch+bugs
Flags: needinfo?(gijskruitbosch+bugs)
Assignee | ||
Comment 16•7 years ago
|
||
Comment on attachment 8829893 [details] [diff] [review] bug_1332595_fix_missing_triggeringprincipal_for_tiles.patch Clearing approval request here for now.
Attachment #8829893 -
Attachment is obsolete: true
Attachment #8829893 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 17•7 years ago
|
||
(In reply to :Gijs from comment #15) > (In reply to Christoph Kerschbaumer [:ckerschb] from comment #14) > > In > > either case, any idea why we would face the problem with > > gSerializationHelper.serializePrincipal [see detailed error message > > underneath and also comment 13]? Is there something we need to udpate or > > can/should we ignore that error message? > > > > > [JavaScript Error: "gSerializationHelper.serializePrincipal is not a function" {file: "chrome://browser/content/browser.js" line: 908} > > The method on the serializationhelper is called serializeToString, not > serializePrincipal. This was my oversight in bug 1331686. :-( I filed bug 1333726 to get this addressed, as it's orthogonal to the patch required here.
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Component: DOM: Security → General
Product: Core → Firefox
Assignee | ||
Comment 19•7 years ago
|
||
In today's episode of: I wish we had a way of having more reliable automated tests, the changes in browser.js cause perma-failure of an about:memory copy-paste test because apparently removing the now-obsolete pagehide code changes how we do focus for some about: pages. https://treeherder.mozilla.org/#/jobs?repo=try&revision=8ed3a4bb740b&selectedJob=71879693 I could reproduce locally, so I fixed the test to wait for the subframe to load before calling .focus(), which seems to fix that issue for me. I think this is a test issue and shouldn't stop us from removing all this bogus unnecessary code from browser.js . :-)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 21•7 years ago
|
||
Comment on attachment 8830243 [details] Bug 1332595 - remove obsolete click handling hacks from browser.js, Clearing review because there's mochitest plain linux bustage that I haven't figured out yet. Probably also focus-related. :-\
Attachment #8830243 -
Flags: review?(dtownsend)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 23•7 years ago
|
||
(In reply to :Gijs from comment #21) > Comment on attachment 8830243 [details] > Bug 1332595 - remove obsolete click handling hacks from browser.js, > > Clearing review because there's mochitest plain linux bustage that I haven't > figured out yet. Probably also focus-related. :-\ Can't repush and then push to try without publishing and re-requesting review. Sigh. Anyway, the code change to browser.js can be reviewed, but I'm not sure about the test change to the reftest-with-caret mochitest yet. I can't reproduce the problem locally. I just know that the analogous fix in the other test worked, so it seems reasonable to expect this fix to work, too. It also looks like that mochitest already has a history of intermittents, so hopefully this will help... :-)
Assignee | ||
Comment 24•7 years ago
|
||
Comment on attachment 8830243 [details] Bug 1332595 - remove obsolete click handling hacks from browser.js, This is still orange on try. :-(
Attachment #8830243 -
Flags: review?(dtownsend)
Assignee | ||
Comment 25•7 years ago
|
||
This is getting ridiculous. Some of the errors seem to be related to artifact mode, but: 1) some also show up for full builds ( https://treeherder.mozilla.org/#/jobs?repo=try&revision=e68cd50f10cefb38c4e5aff3d961180c00e0c883&selectedJob=75475046 ) 2) I can't reproduce locally. 3) I can't reproduce on a one-click loaner running the tests straight from the mozharness test wizard (!) - tests pass fine there. Meanwhile, time is ticking to get this fixed in 53, so I'm going to try to do a more minimal patch and leave getting rid of the rest of the useless code here for a followup, however much that pains me... :-\
Assignee | ||
Comment 26•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5f80c65540a9482e0f6459f820378406247f3e31
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8830243 -
Attachment is obsolete: true
Assignee | ||
Comment 28•7 years ago
|
||
(In reply to :Gijs from comment #26) > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=5f80c65540a9482e0f6459f820378406247f3e31 Seems leaving the pagehide code solves the oranges, so I've split removing that part off to bug 1337794.
Comment 29•7 years ago
|
||
mozreview-review |
Comment on attachment 8834940 [details] Bug 1332595 - remove useless click handling, https://reviewboard.mozilla.org/r/110692/#review112088
Attachment #8834940 -
Flags: review?(dtownsend) → review+
Comment 30•7 years ago
|
||
Pushed by gijskruitbosch@gmail.com: https://hg.mozilla.org/integration/autoland/rev/a03d002ecc4d remove useless click handling, r=mossop
Comment 31•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/a03d002ecc4d
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
Assignee | ||
Comment 32•7 years ago
|
||
Fanolian, can you confirm this is now fixed for you? Should be in today's nightly, I think.
Flags: needinfo?(Fanolian+bugzilla)
Reporter | ||
Comment 33•7 years ago
|
||
(In reply to :Gijs from comment #32) > Fanolian, can you confirm this is now fixed for you? Should be in today's > nightly, I think. It is fixed in 2017-02-09 build. Thanks a lot.
Status: RESOLVED → VERIFIED
Flags: needinfo?(Fanolian+bugzilla)
Assignee | ||
Comment 34•7 years ago
|
||
(In reply to Fanolian from comment #33) > (In reply to :Gijs from comment #32) > > Fanolian, can you confirm this is now fixed for you? Should be in today's > > nightly, I think. > > It is fixed in 2017-02-09 build. Thanks a lot. Thanks! Let's get this uplifted to 53 then.
Assignee | ||
Comment 35•7 years ago
|
||
Comment on attachment 8834940 [details] Bug 1332595 - remove useless click handling, Approval Request Comment [Feature/Bug causing the regression]: bug 1182569 et alii [User impact if declined]: about: links (tiles) on about:newtab are broken [Is this code covered by automated tests?]: The new tab page is covered by automated tests, but the about: links on it aren't. [Has the fix been verified in Nightly?]: yes! [Needs manual test from QE? If yes, steps to reproduce]: wouldn't be a bad idea. Steps: 1) create a bookmark for, say, 'about:crashes' 2) open the new tab page 3) drag the bookmark from the menu / toolbar to the new tab page to create a new pinned tile for that link 4) click the new tile ... and then about:crashes should load, rather than nothing happening. :-) [List of other uplifts needed for the feature/fix]: n/a [Is the change risky?]: not too risky no. [Why is the change risky/not risky?]: Well, it's basically just code removal and if I broke the world I would have expected to hear about it by now. Given that both Mike and me thought this code could go away and Dave concurred, and the automated tests were OK, I suspect we're alright. [String changes made/needed]: no
Attachment #8834940 -
Flags: approval-mozilla-aurora?
Comment 36•7 years ago
|
||
Comment on attachment 8834940 [details] Bug 1332595 - remove useless click handling, Fix for tile functioning, it's had some bake time in m-c, let's bring it to aurora.
Attachment #8834940 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 37•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/5c0b8a190eb7
Updated•7 years ago
|
Comment 38•7 years ago
|
||
I have reproduced this bug with Nightly 53.0a1 (2017-01-20) (64-bit) on Windows 7 , 64 Bit! This bug's fix is verified with latest developer Edition (Aurora)! Build ID : 20170302004002 User Agent : Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:53.0) Gecko/20100101 Firefox/53.0 [testday-20170303]
Assignee | ||
Updated•7 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•