Pass isInContentPage prop to reps

RESOLVED FIXED in Firefox 68

Status

enhancement
P3
normal
RESOLVED FIXED
2 months ago
a month ago

People

(Reporter: nchevobbe, Assigned: nchevobbe)

Tracking

66 Branch
Firefox 68
Dependency tree / graph

Firefox Tracking Flags

(firefox68 fixed)

Details

Attachments

(1 attachment)

Assignee

Description

2 months ago

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.

Priority: -- → P3
Assignee

Comment 1

2 months 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

a month 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

  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)
Assignee

Comment 3

a month ago

(Updated previous comment that was sent to soon)

Assignee

Comment 4

a month 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.

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)
Assignee

Comment 6

a month 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!

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
Assignee

Comment 9

a month 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

a month 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

a month ago
bugherder
Status: ASSIGNED → RESOLVED
Last Resolved: a month ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 68
You need to log in before you can comment on or make changes to this bug.