Closed Bug 1332595 Opened 7 years ago Closed 7 years ago

about:newtab cannot load about:* tiles

Categories

(Firefox :: General, defect, P1)

53 Branch
defect

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)

## 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.
Blocks: 1182569
Has Regression Range: --- → yes
Has STR: --- → yes
See Also: → 1329032, 1331686
[Tracking Requested - why for this release]:
Regression caused by Bug 1182569
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)
Status: UNCONFIRMED → NEW
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.
comment #4 is scary because it suggests we're using the wrong principal entirely somewhere. :-\
Flags: needinfo?(ckerschb)
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]
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)
Attachment #8829893 - Flags: review?(gijskruitbosch+bugs) → review+
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?
(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)
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.
(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)
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
(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)
(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)
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?
(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.
Component: DOM: Security → General
Product: Core → Firefox
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 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)
(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... :-)
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)
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... :-\
Blocks: 1337794
Attachment #8830243 - Attachment is obsolete: true
(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 on attachment 8834940 [details]
Bug 1332595 - remove useless click handling,

https://reviewboard.mozilla.org/r/110692/#review112088
Attachment #8834940 - Flags: review?(dtownsend) → review+
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/a03d002ecc4d
remove useless click handling, r=mossop
https://hg.mozilla.org/mozilla-central/rev/a03d002ecc4d
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
Fanolian, can you confirm this is now fixed for you? Should be in today's nightly, I think.
Flags: needinfo?(Fanolian+bugzilla)
(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)
(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.
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 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+
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]
You need to log in before you can comment on or make changes to this bug.