Pass isInContentPage prop to reps
Categories
(DevTools :: JSON Viewer, enhancement, P3)
Tracking
(firefox68 fixed)
Tracking | Status | |
---|---|---|
firefox68 | --- | fixed |
People
(Reporter: nchevobbe, Assigned: nchevobbe)
References
Details
Attachments
(1 file)
In https://github.com/firefox-devtools/debugger/pull/8170, we added back the href attribute on string links so they can behave as regular link.
In order to do that in a secure way, we only add the href attribute if either the openLink
or the isInContentPage
prop is passed.
For the JSON Viewer, since we're displaying it in the content page, we need to pass the isInContentPage
, and remove any openLink
we may have, in order for the links to be handled by the browser directly.
Updated•5 years ago
|
Assignee | ||
Comment 1•5 years ago
|
||
This will allow links to be handled by the browser, so we
don't have to have our own logic for that.
The reps bundle was updated to reflect the changes made
in Reps files.
Depends on D26710
Assignee | ||
Comment 2•5 years ago
•
|
||
Hello Christoph,
I see a some tab crashes in debug mode with the patch for this bug applied (https://phabricator.services.mozilla.com/D26711).
Steps to reproduce
- Apply the patch
- Navigate to
data:application/json,["visit%20http://mozilla.org"]
- It should open the JSON Viewer, with a mozilla.org link, click on it
Expected results
The link is opened in a new tab
Actual results
A new tab is created but crashes.
It looks like we are failing the assertion from docshell/base/nsDocShell.cpp#9923-9926, that I think you added not long ago.
Would you have any idea why? Thanks!
Assignee | ||
Comment 3•5 years ago
|
||
(Updated previous comment that was sent to soon)
Assignee | ||
Comment 4•5 years ago
|
||
The patch does nothing too fancy IMO. We add an actual href
attribute to the links we produce, and remove the click handler we had before to open said links.
Comment 5•5 years ago
|
||
Hey Nicholas, sorry for the late reply. Do you happen to have a JS stacktrace?
If the assertion |MOZ_ASSERT(nsCSPContext::Equals(csp, argsCSP));| within nsDochsell fires that this indicates that we are not explicitly passing a CSP from JS-land into docshell loads. We added this assertion as pre-requisite because we would like to move the CSP off the Principal and hence we need to ensure wherever we pass a Principal we explicitly pass a Principal alongside with it.
The assertion is nothing to worry for now, because in release code we still use the CSP from the Principal (at least until bug 965637 is fixed). Nevertheless I would like to have it fixed. So if you happen to have a stacktrace I could provide clear guidance on how to fix the problem.
Thanks!
Assignee | ||
Comment 6•5 years ago
|
||
Not sure what you mean by JS stacktrace, as it happens on a link click?
I've recorded the interaction leading to the crash in https://profiler.firefox.com/public/4837a2e8ee15b8a6356a682e504600f76c22538b/stack-chart/?profileName=&published&v=3 , hope it helps!
Comment 7•5 years ago
|
||
FWIW, I can reproduce the problem and I filed Bug 1544534. Thanks for bringing the problem to my attention - I'll take care of it ASAP.
Comment 8•5 years ago
|
||
Nicolas, please add back the checks in browser_jsonview_url_linkification.js that ensured links are real and not fake.
You removed them in https://hg.mozilla.org/mozreview/gecko/rev/1602504731c5a3be4379f9f6631aaf492c7d6daf
Assignee | ||
Comment 9•5 years ago
|
||
(In reply to Oriol Brufau [:Oriol] from comment #8)
Nicolas, please add back the checks in browser_jsonview_url_linkification.js that ensured links are real and not fake.
You removed them in https://hg.mozilla.org/mozreview/gecko/rev/1602504731c5a3be4379f9f6631aaf492c7d6daf
Sure, I added the assertion back in the test.
New push to TRY since Bug 1544534 landed.
Comment 10•5 years ago
|
||
Pushed by nchevobbe@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/113fa9baf3e0 Remove openLink and pass isInContentPage to Rep. r=Honza.
Comment 11•5 years ago
|
||
bugherder |
Description
•