Closed Bug 1144493 Opened 9 years ago Closed 9 years ago

[e10s] clicking the identity popup's More information button displays the Page Info window's General tab, not the Security tab

Categories

(Firefox :: Page Info Window, defect)

39 Branch
defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 40
Tracking Status
e10s m8+ ---
firefox40 --- verified

People

(Reporter: galgeek, Assigned: markh)

References

Details

(Whiteboard: [firefox-ui-tests])

Attachments

(1 file, 1 obsolete file)

With e10s enabled, clicking the identity popup's More information button displays the Page Info window's General tab.

Clicking this button on the identity popup should display the Security tab.
Summary: clicking the identity popup's More information button displays the Page Info window's General tab, not the Security tab → [e10s] clicking the identity popup's More information button displays the Page Info window's General tab, not the Security tab
This is happening across all platforms.

Steps:
1. Load https://bugzilla.mozilla.org
2. Open the door hanger and click on "More Information..."

With step 2 the page info window opens and the security panel has to be selected. This works in non-e10s mode but with e10s enabled it keeps the general tab selected.
tracking-e10s: --- → ?
OS: Mac OS X → All
Hardware: x86 → All
Confirmed e10s, this works as expected in non-e10s mode on latest Nightly.
Whiteboard: [firefox-ui-tests]
In Browser Console:

NS_ERROR_XPC_CANT_PASS_CPOW_TO_NATIVE: It's illegal to pass a CPOW to native code arg 0 [nsIPermissionManager.testExactPermission] SitePermissions.jsm:70:0
This patch seems to make pageinfo work correctly and all related tests pass (ie, it also fixes bug 866413).

The issue is that code tried to use gDocument.documentURIObject and gDocument.nodePrincipal, but give gDocument is a CPOW these are too - and thus they fail when passed to native functions.

I fixed use of gDocument.documentURIObject by replacing uses of that with a function:

> function documentURI() {
>   // gDocument.documentURIObject may be a CPOW - make it a real nsIURI
>   return Services.io.newURI(gDocument.documentURIObject.spec,
>                             gDocument.characterSet,
>                             null);
> }

ie, we just create a new nsIURI based on gDocument.documentURIObject.spec - which seems OK.  The nodePrincipal change is similar - I create a new principal via its URI.spec, ie:

>      // re-create the principal as it may be a CPOW.
>      let principalURI = Services.io.newURI(aPrincipal.URI.spec, null, null);
>      let principalToCheck = Services.scriptSecurityManager.getNoAppCodebasePrincipal(principalURI);

but it's less clear to me that this is OK.

Mike, I'm picking on you because I know how helpful you are ;) If you aren't confident saying if these changes are acceptable or not, could you please request feedback from someone you think would be?
Attachment #8598370 - Flags: feedback?(mconley)
Comment on attachment 8598370 [details] [diff] [review]
0007-Bug-1144493-make-page-info-work-with-e10s.-r.patch

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

As a wallpaper-over fix, this is probably fine.

What really needs to happen, is that the pageInfo dialog needs to be re-written to not interact directly with the content.

That's a bigger project, likely going to take place in bug 1040947 somewhere down the line.

But I think we should take something like this for now until bug 1040947 can get some attention.

::: browser/base/content/pageinfo/permissions.js
@@ +29,5 @@
>  };
>  
>  function onLoadPermission()
>  {
> +  var uri = documentURI();

I'd just use BrowserUtils.makeURIFromCPOW, which is functionally equivalent, and easier to trace when we start tearing it out.

::: browser/modules/Feeds.jsm
@@ +39,5 @@
>  
>      if (aIsFeed) {
> +      // re-create the principal as it may be a CPOW.
> +      let principalURI = Services.io.newURI(aPrincipal.URI.spec, null, null);
> +      let principalToCheck = Services.scriptSecurityManager.getNoAppCodebasePrincipal(principalURI);

A quick DXR shows that this _is_ okay to do - though again, BrowserUtils.makeURIFromCPOW is probably your friend here.
Attachment #8598370 - Flags: feedback?(mconley) → feedback+
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #5)
> But I think we should take something like this for now until bug 1040947 can
> get some attention.

SGTM!

> I'd just use BrowserUtils.makeURIFromCPOW, which is functionally equivalent,
> and easier to trace when we start tearing it out.
...
> A quick DXR shows that this _is_ okay to do - though again,
> BrowserUtils.makeURIFromCPOW is probably your friend here.

Done.

Try at https://treeherder.mozilla.org/#/jobs?repo=try&revision=69ea4f93ff90
Assignee: nobody → mhammond
Attachment #8598370 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8599636 - Flags: review?(mconley)
Comment on attachment 8599636 [details] [diff] [review]
0006-Bug-1144493-wallpaper-over-enough-cracks-to-make-pag.patch

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

Yeah, this should do for now.

Really looking forward to this getting re-written to use messages instead.

::: browser/base/content/test/plugins/browser.ini
@@ +44,5 @@
>    plugin_crashCommentAndURL.html
>    plugin_zoom.html
>  
>  [browser_bug743421.js]
> +skip-if = e10s

We should file a bug(s) to re-enable these, if one doesn't already exist.
Attachment #8599636 - Flags: review?(mconley) → review+
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #7)
> We should file a bug(s) to re-enable these, if one doesn't already exist.

Looks like that was already done in bug 1129040.

Try at https://treeherder.mozilla.org/#/jobs?repo=try&revision=9f748b91b608

https://hg.mozilla.org/integration/fx-team/rev/a9878c3dd8a9
https://hg.mozilla.org/mozilla-central/rev/a9878c3dd8a9
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 40
Verified with Nightly build from 20150505
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.