1352778 - F5 and other normal ways to reload the page don't work with local .json file opened in JSON viewer

Status: VERIFIED FIXED

(DevTools :: JSON Viewer, defect, P3)

Description

[684sigma]

I found a bug in Firefox Beta 53. It also happens in Nightly 55. Doesn't happen in Beta 52, because it's a new feature in Beta 53. But it worked well, before I updated from Beta 53b5 to Beta 53b8. Therefore I'm adding keyword "regression".
The bug is: Pressing Ctrl+R / F5, clicking reload button, or "Reload" menuitem doesn't reload local .json file opened in JSON viewer. I noticed this bug when I changed something in a local .json file and tried to reload it in browser to check the result.
Here's how to reproduce it:

1. Download attachment 8853723 [details] and open it in a new tab
2. Press F5

Result: F5 (and other mentioned things) has no effect
Expected: F5 works perfectly

Comment 1

[Loic]

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=b771ead300e82a2e2f0a63d01a3e5256de96f126&tochange=2bf09c87b198b9b59ed84bc1bc76236fa1d9e972

Regressed by Bug 1333210.

Comment 2

[:Gijs (he/him)]

> Security Error: Content at moz-nullprincipal:{44f6f526-8a01-b248-b2f3-9e18ed863ace} may not load or link to file:///.../blob.json

Christoph, is there a sane way around this problem?

Comment 3

[Christoph Kerschbaumer [:ckerschb}]

(In reply to :Gijs from comment #2)
> > Security Error: Content at moz-nullprincipal:{44f6f526-8a01-b248-b2f3-9e18ed863ace} may not load or link to file:///.../blob.json
> 
> Christoph, is there a sane way around this problem?

Gijs, I took a closer a look at this problem, so let me summarize: The initial load of the JSON file uses a triggeringprincipal of Systemprincipal but then gets overruled by [1] which forces all the principals in the loadInfo to be reset to a freshly created  NullPrincipal. In turn this resetting of principals causes the history entry to be created using a NullPrincipal as the triggeringPrincipal which then obviously is not allowed to load a file:// URI.

The fact that we have a principalToInherit which in this case would and should be a NullPrincipal makes me wonder if resetting the triggeringPrincipal here [2] is even correct. Probably we should *not* reset the triggeringPrincipal. In my opinion it should remain the same. In the case here it would remain the SystemPrincipal which actually triggered the initial load of the JSON which would also fix the problem described in this bug.

With that changed the test landed within Bug 1333210 still works and we would have to update test_forceinheritprincipal_overrule_owner.html to reflect those changes.


[1] https://hg.mozilla.org/releases/mozilla-aurora/rev/8d73c486834d#l1.15
[2] https://hg.mozilla.org/mozilla-central/rev/3b7254b9fe5f#l2.71

Comment 4

[:Gijs (he/him)]

Makes sense, though all this stuff is nettly. Boris, does comment #3 make sense to you, too?

Comment 5

[684sigma]

There's another very similar bug: Browser shows empty page when restoring tab with local .json file
1. Download attachment 8853723 [details] and open it in a new tab
2. Close tab, restore tab

I haven't filed it yet, but probably connected. I see the same error in browser console:
Security Error: Content at moz-nullprincipal:{964d6986-b3b0-4a0f-ba4a-e23fe748e785} may not load or link to file:///.../example%20of%20JSON%20file,%20by%20reporter.json.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Yes, I don't see why we would reset anything _other_ than the principalToInherit....

Comment 7

[Christoph Kerschbaumer [:ckerschb}]

Attachment: bug_1352778_reset_triggeringPrincipal.patch

I fixed that up, and I think we should uplift that change.

I verified that:
*) STRs from comment 0 work again
*) test/browser_jsonview_valid_json.js  works correctly
*) test_forceinheritprincipal_overrule_owner.html works correctly
*) STRs from comment 5 work again (but someone should verify once again)

Comment 8

[:Gijs (he/him)]

Comment on attachment 8854093 [details] [diff] [review]
bug_1352778_reset_triggeringPrincipal.patch

Review of attachment 8854093 [details] [diff] [review]:
-----------------------------------------------------------------

r=me

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1f8f3c6fecd9
Do not reset triggeringPrincipal but only principalToInherit within loadInfo when forced to. r=gijs

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/1f8f3c6fecd9

Comment 11

[Liz Henry (:lizzard) (relman/hg->git project)]

Are you sure it is unaffected for 52? If it's caused by bug 1333210 that landed for 52.

Comment 12

[Loic]

I forget regularly to set flags for ESR, and I don't have access to bug 1333210, but ESR is probably affected too. :)

NB/ someone can add bug 1333210 to the "blocks" field, please.

Comment 13

[:Gijs (he/him)]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #11)
> Are you sure it is unaffected for 52? If it's caused by bug 1333210 that
> landed for 52.

The JSON viewer is disabled by default on 52.

Comment 14

[Christoph Kerschbaumer [:ckerschb}]

Gijs, can I convince you to fill out the uplift request?

Comment 15

[:Gijs (he/him)]

Comment on attachment 8854093 [details] [diff] [review]
bug_1352778_reset_triggeringPrincipal.patch

Approval Request Comment
[Feature/Bug causing the regression]: security issue with the JSON viewer
[User impact if declined]: no obvious way to reload JSON files once they show in the viewer, potentially other, similar issues with the viewer
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: not yet
[Needs manual test from QE? If yes, steps to reproduce]: see comment #0
[List of other uplifts needed for the feature/fix]: n/a
[Is the change risky?]: uh, well, it's the last week of beta, so inasmuch as changes are normally non-risky in the last week of beta...
[Why is the change risky/not risky?]: We're renaming an XPCOM API and changing what it does. The API was new in 52, and no add-ons use it, so the API change should be safe externally. For the actual effect of the change, it should be limited to the JSON viewer because in-tree, it is currently the only (non-test) consumer of this API. We have tests verifying the API works as advertised, and we have separate tests that the effect on the json viewer is as it should be, so given that those pass, I think that's all the areas I can think of for regressions covered.
[String changes made/needed]: no

Comment 16

[:Gijs (he/him)]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #14)
> Gijs, can I convince you to fill out the uplift request?

Done. Can you file a followup bug to create an automated regression test for comment #0 and potentially comment #5? Thanks!

Comment 17

[Christoph Kerschbaumer [:ckerschb}]

(In reply to :Gijs from comment #16)
> Can you file a followup bug to create an automated regression test for
> comment #0 and potentially comment #5? Thanks!

I filed Bug 1353736 to get that done.

Comment 18

[Liz Henry (:lizzard) (relman/hg->git project)]

Too late for a fix for 53, minor carryover regression, no one assigned, so these don't seem like a high priority.

Comment 19

[Gerry Chang [:gchang]]

Hi Brindusa, could you help find someone to verify if this issue was fixed as expected on a latest Nightly build? Thanks!

Comment 20

[roxleitan]

Verified fixed using the latest Nightly 55.0a1 (2017-04-07) on Windows 10, Ubuntu 16.04 and Mac OS X 10.10.

Comment 21

[Gerry Chang [:gchang]]

Comment on attachment 8854093 [details] [diff] [review]
bug_1352778_reset_triggeringPrincipal.patch

Fix a regression related to reload JSON files in the viewer and was verified. Aurora54+.

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/fbe4c41963bc

Comment 23

[Emil Ghitta, QA [:emilghitta]]

I’ve reproduced the issue described in comment 0 using Firefox  55.0a1 (Build Id:20170401030204) and verified that the issue is not reproducible anymore using Firefox 54.0b1 (Build Id: 20170420011827) on Windows 10 64bit, macOS 10.11.6 and Ubuntu 16.04 64bit.