Closed Bug 1238180 Opened 8 years ago Closed 8 years ago

Unsafe CPOW usage in nsContextMenu.js, browser.js for pageInfo opened from context menu

Categories

(Firefox :: Page Info Window, defect)

Product:
Firefox
Component:
Page Info Window
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 46
Tracking Flags:
Tracking Status
e10s m8+ ---
firefox45 --- fixed
firefox46 --- fixed

People

(Reporter: arni2033, Assigned: mconley)

References

Details

Attachments

(1 file)

MozReview Request: Bug 1238180 - Avoid unsafe CPOWs when opening Page / Frame / Image Info from the context menu. r=florian
58 bytes, text/x-review-board-request
florian
: review+
Sylvestre
: approval-mozilla-aurora+
Details 
>>>   My Info:   Win7_64, Nightly 46, 32bit, ID 20160107030235
STR:
0. Start browser in e10s-mode, open Browser Console
1. Open page   data:text/html,<body></body>
2. Right-click on content area, click "View Page Info" menuitem

Result:       4 CPOW usages in nsContextMenu.js, 1 CPOW usage in browser.js
Expectations: No CPOW

Note:
 The closest thing I found is bug 1170291.
 I detected this CPOW usage with extension "PKE-meter" (created in bug 1109713)
tracking-e10s: --- → ?
Assignee: nobody → mconley
tracking-e10s: ? → m8+
Summary: Unsafe CPOW usage in nsContextMenu.js, browser.js for pageInfo opened from context menu → Unsafe CPOW usage in nsContextMenu.js, browser.js for pageInfo for data URIs opened from context menu
Summary: Unsafe CPOW usage in nsContextMenu.js, browser.js for pageInfo for data URIs opened from context menu → Unsafe CPOW usage in nsContextMenu.js, browser.js for pageInfo opened from context menu
The same happens on http://example.org/ . Do request a screencast if it's necessary.
Comment on attachment 8709088 [details]
MozReview Request: Bug 1238180 - Avoid unsafe CPOWs when opening Page / Frame / Image Info from the context menu. r=florian

https://reviewboard.mozilla.org/r/31255/#review28139

::: browser/base/content/nsContextMenu.js:1068
(Diff revision 1)
> -    BrowserPageInfo(this.target.ownerDocument.defaultView.top.document);
> +    BrowserPageInfo(gContextMenuContentData.topDocLocation);

Would anything break if we just gave null as the document URL when the document is the top level one?

It seems strange that we need to transfer the top document URL from the content process.
Attachment #8709088 - Flags: review?(florian)
Whiteboard: [e10s-45-uplift]
https://reviewboard.mozilla.org/r/31255/#review28139

> Would anything break if we just gave null as the document URL when the document is the top level one?
> 
> It seems strange that we need to transfer the top document URL from the content process.

I guess I'm just trying to keep the status-quo here; we used to pass the top-level document URI, and now we're still doing that.

From a cursory glance at the code, we use the document URI to uniquely identify the Page Info window so that if we attempt to view Page Info for the same page, we re-use the same window. Using the top document URI is, I guess, for the case where the user has right-clicked on "View Page Info" as opposed to "This Frame > View Frame Info".

I guess we might be able to get away with passing in null in this case - we'd end up defaulting to using the window's current browser URI. Would you prefer that?
ni for comment 4
Flags: needinfo?(florian)
Now that I'm looking at this again, I think this.target.ownerDocument.defaultView.top.document was written specifically to avoid using gBrowser so that things would work even when using the context menu in the sidebar. Does your current patch work in that case? If so I'll r+.

Note: unfortunately, I forgot to think about the sidebar case when reviewing bug 1040947, so I let a line using gBrowser be added http://hg.mozilla.org/mozilla-central/rev/b32f21db299c#l5.131 :-(.
Flags: needinfo?(florian)
Yeah, this breaks the sidebar case (which doesn't appear to be hooked up to gContextMenuContentData). I think we should take this regression against the sidebar browser though, especially seeing as how bug 1233497 will be landing shortly, which will break the Page Info dialog for normal browser tabs without this patch.

You concurred with this plan in IRC, so r=you?
Flags: needinfo?(florian)
So if the sidebar case is broken either way, we can pass null as the document URL, right?
Flags: needinfo?(florian)
(In reply to Florian Quèze [:florian] [:flo] from comment #8)
> So if the sidebar case is broken either way, we can pass null as the
> document URL, right?

Yes, that's true! I'll update the patch.
Comment on attachment 8709088 [details]
MozReview Request: Bug 1238180 - Avoid unsafe CPOWs when opening Page / Frame / Image Info from the context menu. r=florian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31255/diff/1-2/
Attachment #8709088 - Flags: review?(florian)
Comment on attachment 8709088 [details]
MozReview Request: Bug 1238180 - Avoid unsafe CPOWs when opening Page / Frame / Image Info from the context menu. r=florian

https://reviewboard.mozilla.org/r/31255/#review28361

Thanks for the updated patch!

::: browser/base/content/nsContextMenu.js:1068
(Diff revision 2)
> -    BrowserPageInfo(this.target.ownerDocument.defaultView.top.document);
> +    BrowserPageInfo(null);

You can just omit the parameter here.
Attachment #8709088 - Flags: review?(florian) → review+
https://reviewboard.mozilla.org/r/31255/#review28361

> You can just omit the parameter here.

Done, thanks!
Comment on attachment 8709088 [details]
MozReview Request: Bug 1238180 - Avoid unsafe CPOWs when opening Page / Frame / Image Info from the context menu. r=florian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31255/diff/2-3/
Attachment #8709088 - Attachment description: MozReview Request: Bug 1238180 - Avoid unsafe CPOWs when opening Page / Frame / Image Info from the context menu. r?florian → MozReview Request: Bug 1238180 - Avoid unsafe CPOWs when opening Page / Frame / Image Info from the context menu. r=florian
https://hg.mozilla.org/integration/fx-team/rev/69c213447aeefd9b1a464d94fa3a37807978becc
Bug 1238180 - Avoid unsafe CPOWs when opening Page / Frame / Image Info from the context menu. r=florian
Comment on attachment 8709088 [details]
MozReview Request: Bug 1238180 - Avoid unsafe CPOWs when opening Page / Frame / Image Info from the context menu. r=florian

Approval Request Comment
[Feature/regressing bug #]:

e10s

[User impact if declined]:

Users will be using unsafe CPOWs when opening up Page Info via the context menu. This can hamper performance. At the very least, "unsafe CPOW usage" will be emitted in the Browser Console.

Note that this patch will break opening Page Info the sidebar browser (which is a rather obscure feature that is not exposed by default). Page Info was already broken with the sidebar browser (it wouldn't show any actual page info), but it would at least open. Now clicking on "View Page Info" in the context menu of the sidebar browser will result in nothing occurring. Note that there are already a number of things that are broken in the sidebar browser (like "Inspect Element"). I will be filing a follow-up bug to see if we should fix the sidebar browser cases, or to find out whether or not sidebar browser is going to go into the "dead" group of "great or dead".

[Describe test coverage new/current, TreeHerder]:

There are several tests that open the page info window for browser tabs, all of which are passing.

[Risks and why]: 

I don't see much risk here. Note that View Page Info was already broken for the sidebar browser, since it wouldn't actually show any information.

[String/UUID change made/needed]:

None.
Attachment #8709088 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/69c213447aee
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox46: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46
Attachment #8709088 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Filed bug 1241892 for the sidebar browser breakage.
Depends on: 1241892
[bugday-20160323]

Status: RESOLVED,FIXED -> UNVERIFIED

Comments:
STR: Not clear.
Developer specific testing

Component: 
Name			Firefox
Version			46.0b9
Build ID		20160322075646
Update Channel          beta
User Agent		Mozilla/5.0 (Windows NT 6.1; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0
OS			Windows 7 SP1 x86_64

Expected Results: 
Developer specific testing

Actual Results: 
As expected
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: