Closed Bug 1540069 Opened 5 years ago Closed 5 years ago

Pass isInContentPage prop to reps

Categories

(DevTools :: JSON Viewer, enhancement, P3)

66 Branch
enhancement

Tracking

(firefox68 fixed)

RESOLVED FIXED
Firefox 68
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.

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

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

  1. Apply the patch
  2. Navigate to data:application/json,["visit%20http://mozilla.org"]
  3. 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!

Flags: needinfo?(ckerschb)

(Updated previous comment that was sent to soon)

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.

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!

Flags: needinfo?(ckerschb) → needinfo?(nchevobbe)

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!

Flags: needinfo?(nchevobbe)

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.

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

Blocks: 1447820

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

Pushed by nchevobbe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/113fa9baf3e0
Remove openLink and pass isInContentPage to Rep. r=Honza.
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 68
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: