"Get Add-ons" pane is being restricted by X-FRAME-OPTIONS

VERIFIED FIXED in mozilla2.0b7

Status

()

Core
DOM
--
major
VERIFIED FIXED
7 years ago
7 years ago

People

(Reporter: KWierso, Assigned: bsterne)

Tracking

Trunk
mozilla2.0b7
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +
in-litmus -

Firefox Tracking Flags

(blocking2.0 betaN+)

Details

(Whiteboard: [4b5])

Attachments

(3 attachments, 3 obsolete attachments)

(Reporter)

Description

7 years ago
User-Agent:       Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b6pre) Gecko/20100903 Firefox/4.0b6pre
Build Identifier: 

Looks like it doesn't like being loaded in an iframe. I see an error page on screen, and a really long error logged to the error console (due to all of my extensions being parts of the requested URL?).

Reproducible: Always

Steps to Reproduce:
1. Open Addons Manager
2. Go to "Get Addons" pane
3. See the error.
Actual Results:  
Content from AMO is restricted by X-FRAME-OPTIONS header.

Expected Results:  
Content from AMO is displayed in the pane.

X-FRAME-OPTIONS: blocked https://services.addons.mozilla.org/en-US/firefox/discovery/4.0b6pre/WINNT#{%22CompactMenuCE@Merci.chao%22:{%22name%22:%22Personal%20Menu%22,%22version%22:%224.3.2%22,%22type%22:%22extension%22,%22userDisabled%22:true,%22isCompatible%22:false,%22isBlocklisted%22:false},%22contacts@labs.mozilla.com%22:{%22name%22:%22Contacts%22,%22version%22:%220.3.2%22,%22type%22:%22extension%22,%22userDisabled%22:true,%22isCompatible%22:false,%22isBlocklisted%22:false},%22dataman@kairo.at%22:{%22name%22:%22Data%20Manager%22,%22version%22:%221.0.1%22,%22type%22:%22extension%22,%22userDisabled%22:true,%22isCompatible%22:false,%22isBlocklisted%22:false},%22downloadbadge@jes_jm.software%22:{%22name%22:%22Download%20Badge%22,%22version%22:%220.1.7%22,%22type%22:%22extension%22,%22userDisabled%22:true,%22isCompatible%22:false,%22isBlocklisted%22:false},%22firebug@software.joehewitt.com%22:{%22name%22:%22Firebug%22,%22version%22:%221.6X.0b1%22,%22type%22:%22extension%22,%22userDisabled%22:false,%22isCompatible%22:true,%22isBlocklisted%22:false},%22inspector@mozilla.org%22:{%22name%22:%22DOM%20Inspector%22,%22version%22:%222.0.8%22,%22type%22:%22extension%22,%22userDisabled%22:false,%22isCompatible%22:true,%22isBlocklisted%22:false},%22jid0-t3eeRQgGANLCH9c50lPqcTDuNng@jetpack%22:{%22name%22:%22Add-ons%20Builder%20Helper%22,%22version%22:%220.0.5%22,%22type%22:%22extension%22,%22userDisabled%22:false,%22isCompatible%22:false,%22isBlocklisted%22:false},%22locationbar2@design-noir.de%22:{%22name%22:%22Locationbar%C2%B2%22,%22version%22:%221.0.5%22,%22type%22:%22extension%22,%22userDisabled%22:true,%22isCompatible%22:false,%22isBlocklisted%22:false},%22omnibar@ajitk.com%22:{%22name%22:%22Omnibar%22,%22version%22:%220.7.0.20100707%22,%22type%22:%22extension%22,%22userDisabled%22:false,%22isCompatible%22:false,%22isBlocklisted%22:false},%22rtse-nightly@shawnwilsher.com%22:{%22name%22:%22RT%20Site%20Extender%22,%22version%22:%221.2.0.20100829%22,%22type%22:%22extension%22,%22userDisabled%22:false,%22isCompatible%22:true,%22isBlocklisted%22:false},%22siteprefs.mehdi@mozilla.com%22:{%22name%22:%22Site%20Preferences%22,%22version%22:%220.1.2%22,%22type%22:%22extension%22,%22userDisabled%22:true,%22isCompatible%22:false,%22isBlocklisted%22:false},%22tabsintitlebar@roosterteethsiteextender.com%22:{%22name%22:%22Tabs%20in%20Titlebar%22,%22version%22:%220.1%22,%22type%22:%22extension%22,%22userDisabled%22:true,%22isCompatible%22:true,%22isBlocklisted%22:false},%22testpilot@labs.mozilla.com%22:{%22name%22:%22Test%20Pilot%22,%22version%22:%221.0.1%22,%22type%22:%22extension%22,%22userDisabled%22:false,%22isCompatible%22:false,%22isBlocklisted%22:false},%22twitternotifier@naan.net%22:{%22name%22:%22Echofon%22,%22version%22:%221.9.6.5RC2%22,%22type%22:%22extension%22,%22userDisabled%22:false,%22isCompatible%22:true,%22isBlocklisted%22:false},%22{02450954-cdd9-410f-b1da-db804e18c671}%22:{%22name%22:%22Screengrab%22,%22version%22:%220.96.3%22,%22type%22:%22extension%22,%22userDisabled%22:false,%22isCompatible%22:false,%22isBlocklisted%22:false},%22{1280606b-2510-4fe0-97ef-9b5a22eafe80}%22:{%22name%22:%22Console%C2%B2%22,%22version%22:%220.6.1%22,%22type%22:%22extension%22,%22userDisabled%22:true,%22isCompatible%22:false,%22isBlocklisted%22:false},%22{2e61e246-e640-4c56-b1ed-f146dbed48cd}%22:{%22name%22:%22Stop%20Autoplay%22,%22version%22:%220.7.6%22,%22type%22:%22extension%22,%22userDisabled%22:true,%22isCompatible%22:false,%22isBlocklisted%22:false},%22{46551EC9-40F0-4e47-8E18-8E5CF550CFB8}%22:{%22name%22:%22Stylish%22,%22version%22:%221.0.11%22,%22type%22:%22extension%22,%22userDisabled%22:false,%22isCompatible%22:false,%22isBlocklisted%22:false},%22{59c81df5-4b7a-477b-912d-4e0fdf64e5f2}%22:{%22name%22:%22ChatZilla%22,%22version%22:%220.9.86%22,%22type%22:%22extension%22,%22userDisabled%22:false,%22isCompatible%22:false,%22isBlocklisted%22:false},%22{d10d0bf8-f5b5-c8b4-a8b2-2b9879e08c5d}%22:{%22name%22:%22Adblock%20Plus%22,%22version%22:%221.3a.20100831%22,%22type%22:%22extension%22,%22userDisabled%22:false,%22isCompatible%22:true,%22isBlocklisted%22:false},%22{e4a8a97b-f2ed-450b-b12d-ee082ba24781}%22:{%22name%22:%22Greasemonkey%22,%22version%22:%220.8.20100408.6.3%22,%22type%22:%22extension%22,%22userDisabled%22:false,%22isCompatible%22:true,%22isBlocklisted%22:false},%22{000a9d1c-beef-4f90-9363-039d445309b8}%22:{%22name%22:%22Google%20Gears%22,%22version%22:%220.5.36.0%22,%22type%22:%22extension%22,%22userDisabled%22:true,%22isCompatible%22:false,%22isBlocklisted%22:false},%22{3252b9ae-c69a-4eaf-9502-dc9c1f6c009e}%22:{%22name%22:%22Default%20Manager%22,%22version%22:%222.2%22,%22type%22:%22extension%22,%22userDisabled%22:true,%22isCompatible%22:false,%22isBlocklisted%22:false},%22{27182e60-b5f3-411c-b545-b44205977502}%22:{%22name%22:%22Search%20Helper%20Extension%22,%22version%22:%221.0%22,%22type%22:%22extension%22,%22userDisabled%22:true,%22isCompatible%22:false,%22isBlocklisted%22:false},%22{73a6fe31-595d-460b-a920-fcc0f8843232}%22:{%22name%22:%22NoScript%22,%22version%22:%222.0.2.5rc1%22,%22type%22:%22extension%22,%22userDisabled%22:false,%22isCompatible%22:true,%22isBlocklisted%22:false},%22{972ce4c6-7e08-4474-a285-3208198ce6fd}%22:{%22name%22:%22Default%22,%22version%22:%224.0b6pre%22,%22type%22:%22theme%22,%22userDisabled%22:false,%22isCompatible%22:true,%22isBlocklisted%22:false},%2233@personas.mozilla.org%22:{%22name%22:%22Groovy%20Blue%22,%22version%22:%22%22,%22type%22:%22theme%22,%22userDisabled%22:true,%22isCompatible%22:true,%22isBlocklisted%22:false},%22215507@personas.mozilla.org%22:{%22name%22:%22Firefox%20Cup%20-%20USA%22,%22version%22:%22%22,%22type%22:%22theme%22,%22userDisabled%22:true,%22isCompatible%22:true,%22isBlocklisted%22:false},%22{06b38f9d-7392-47a0-b3de-bb9304e66837}%22:{%22name%22:%22Microsoft%C2%AE%20Windows%20Media%20Player%20Firefox%20Plugin%22,%22version%22:%221.0.0.8%22,%22type%22:%22plugin%22,%22userDisabled%22:false,%22isCompatible%22:true,%22isBlocklisted%22:false},%22{9b29438d-3436-4cc8-821e-d2cf929caa9c}%22:{%22name%22:%22Foxit%20Reader%20Plugin%20for%20Mozilla%22,%22version%22:%221.0.1.1117%22,%22type%22:%22plugin%22,%22userDisabled%22:false,%22isCompatible%22:true,%22isBlocklisted%22:false},%22{776c1d62-6afa-49e3-ba60-6c8e02129836}%22:{%22name%22:%22Google%20Update%22,%22version%22:%221.2.183.29%22,%22type%22:%22plugin%22,%22userDisabled%22:false,%22isCompatible%22:true,%22isBlocklisted%22:false},%22{6a5396e5-ae37-4c8e-b7bc-7d9a2ac0fed7}%22:{%22name%22:%22Shockwave%20Flash%22,%22version%22:%2210.1.82.76%22,%22type%22:%22plugin%22,%22userDisabled%22:false,%22isCompatible%22:true,%22isBlocklisted%22:false},%22{ea5854ac-49c9-4532-97c0-1216098104c4}%22:{%22name%22:%22Garmin%20Communicator%20Plug-In%22,%22version%22:%222.8.1.0%22,%22type%22:%22plugin%22,%22userDisabled%22:false,%22isCompatible%22:true,%22isBlocklisted%22:false},%22{f723e9b0-4dff-48f1-8e4e-76154c57abe6}%22:{%22name%22:%22Java(TM)%20Platform%20SE%206%20U21%22,%22version%22:%226.0.210.7%22,%22type%22:%22plugin%22,%22userDisabled%22:false,%22isCompatible%22:true,%22isBlocklisted%22:false},%22{cacaa5d6-9843-4b9e-b013-8ec3c3247ec4}%22:{%22name%22:%22Java%20Deployment%20Toolkit%206.0.210.7%22,%22version%22:%226.0.210.7%22,%22type%22:%22plugin%22,%22userDisabled%22:false,%22isCompatible%22:true,%22isBlocklisted%22:false},%22{76952b67-64de-4320-9cd2-2f2dd392ef84}%22:{%22name%22:%22Silverlight%20Plug-In%22,%22version%22:%224.0.50524.0%22,%22type%22:%22plugin%22,%22userDisabled%22:false,%22isCompatible%22:true,%22isBlocklisted%22:false},%22{6870bba4-7f50-4676-8fb2-784bcccee91b}%22:{%22name%22:%22Microsoft%20Office%202010%22,%22version%22:%2214.0.4730.1010%22,%22type%22:%22plugin%22,%22userDisabled%22:false,%22isCompatible%22:true,%22isBlocklisted%22:false},%22{b9284f3e-b0fc-4115-9c49-eadfc63c47a6}%22:{%22name%22:%22Microsoft%20Office%202010%22,%22version%22:%2214.0.4761.1000%22,%22type%22:%22plugin%22,%22userDisabled%22:false,%22isCompatible%22:true,%22isBlocklisted%22:false},%22{0410798f-2e26-40f7-bd38-c1cfd461dc57}%22:{%22name%22:%22Microsoft%20Office%20Live%20Plug-in%20for%20Firefox%22,%22version%22:%222.0.3009.0%22,%22type%22:%22plugin%22,%22userDisabled%22:false,%22isCompatible%22:true,%22isBlocklisted%22:false},%22{e1274911-3d4c-483d-a5c5-89d15abe4ec6}%22:{%22name%22:%22Windows%20Live%C2%99%20Photo%20Gallery%22,%22version%22:%2215.4.3002.810%22,%22type%22:%22plugin%22,%22userDisabled%22:false,%22isCompatible%22:true,%22isBlocklisted%22:false},%22{f6df2a89-4624-4cc4-a3af-8724e8dc6c81}%22:{%22name%22:%22VLC%20Multimedia%20Plug-in%22,%22version%22:%221.0.5.0%22,%22type%22:%22plugin%22,%22userDisabled%22:false,%22isCompatible%22:true,%22isBlocklisted%22:false}}
This feels like a bug in the X-FRAME-OPTIONS implementation, since we are a
chrome privileged page loading content in browser element.
Blocks: 584831
Blocks last beta I think, though I think it'd be really really good to see a fix somehow for b6, maybe involving removing the header from AMO till this is done.
blocking2.0: --- → betaN+
Duplicate of this bug: 593346
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows 7 → All
Hardware: x86 → All
Version: unspecified → Trunk
(Assignee)

Comment 4

7 years ago
Taking this.  Will have a patch shortly.
Assignee: nobody → bsterne
Sadly this prevents us also in Beta5 and down to Beta3 to show the one and only location where the Addons Compatibility Reporter is promoted. I would expect more negative via feedback.
Assignee: bsterne → nobody
Whiteboard: [4b5]
Assignee: nobody → bsterne
(Reporter)

Comment 6

7 years ago
Created attachment 471899 [details]
screenshot

Oddly, if the "Get Add-ons" pane is pre-selected when the addon manager loads (previously open the AOM, click "get addons", close the tab, reopen the tab), I get this page loading.
(In reply to comment #6)
> Created attachment 471899 [details]
> screenshot
> 
> Oddly, if the "Get Add-ons" pane is pre-selected when the addon manager loads
> (previously open the AOM, click "get addons", close the tab, reopen the tab), I
> get this page loading.

That is the unrelated bug 584693
(Assignee)

Comment 8

7 years ago
Created attachment 471922 [details] [diff] [review]
fix

Add a system principal check.  Boris, perhaps you can tell me the most efficient way to get a principal from a nsIDOMWindow.  I'll also add a test.
Attachment #471922 - Flags: review?(bzbarsky)
Comment on attachment 471922 [details] [diff] [review]
fix

>+    nsCOMPtr<nsIDOMDocument> topDOMDoc;
>+    topWindow->GetDocument(getter_AddRefs(topDOMDoc));
>+    nsCOMPtr<nsINode> node = do_QueryInterface(topDOMDoc);
>+    nsCOMPtr<nsIPrincipal> topPrincipal = node->NodePrincipal();

  nsCOMPtr<nsIScriptObjectPrincipal> objPrin = do_QueryInterface(topWindow);
  nsIPrincipal* topPrincipal = objPrin->GetPrincipal();

>+    if (nsContentUtils::IsSystemPrincipal(topPrincipal)) {

This should be a subsumes() check.

Or more interest to me....  Why is this a problem at all?  Are we seriously loading content off the web in a chrome-type iframe, and if so, why?
Attachment #471922 - Flags: review?(bzbarsky) → review-
(Assignee)

Comment 10

7 years ago
Created attachment 471988 [details] [diff] [review]
fix v2

(In reply to comment #9)
> Or more interest to me....  Why is this a problem at all?  Are we seriously
> loading content off the web in a chrome-type iframe, and if so, why?

The Add-ons Manager folks will have to speak to that, but in the meantime here's the patch with the Subsumes check.

I'm still having trouble with the test, but I'll see if I can get attached to the bug relatively soon.
Attachment #471922 - Attachment is obsolete: true
Attachment #471988 - Flags: review?(bzbarsky)
Oh, wait.  I guess this is about the add-ons-manager-in-browser-tab case? In that situation, I can see why this is being a problem....
Two comments on the patch:

1)  If a chrome page in a content docshell loads site A which then frames site B, the framing will be allowed with this patch.  Is this desired?

2)  Getting the principal of the current document can just be done with
    NodePrincipal(); no need to get it off thisWindow.
Duplicate of this bug: 593819
Comment on attachment 471988 [details] [diff] [review]
fix v2

Pending answers to the questions.
Attachment #471988 - Flags: review?(bzbarsky) → review-
(Assignee)

Comment 15

7 years ago
(In reply to comment #12)
> Two comments on the patch:
> 1)  If a chrome page in a content docshell loads site A which then frames site
> B, the framing will be allowed with this patch.  Is this desired?

Good point.  I'm not sure how often we have chrome top-level page framing web content, but this would happen in those cases.  Perhaps I should get DocShells and call GetSameTypeRootTreeItem()?

> 2)  Getting the principal of the current document can just be done with
>     NodePrincipal(); no need to get it off thisWindow.

Yep, I'll fix this for the next version of the patch.
> Perhaps I should get DocShells and call GetSameTypeRootTreeItem()?

I'm not sure what you mean; what policy are you trying to enforce?
(Assignee)

Comment 17

7 years ago
X-Frame-Options 1) restricts "DENY" documents from being framed in any context and 2) restricts "SAMEORIGIN" documents to be framed only if the top-level browsing context is the same origin as the document.

This bug is about adding exceptions to the above policy for chrome.  We want to allow chrome to frame any resource but we don't want to allow content to frame other content if it would violate any of the above policy.

My thinking about GetSameTypeRootTreeItem is:
1) any inner content document with X-F-O: DENY should be blocked if there exists a content docshell anywhere in its parent chain
2) any inner content document with X-F-O: SAMEORIGIN should be blocked if the top content docshell in its parent chain is a different origin
3) we don't need to worry about the "content > chrome > content" chain because we don't allow content to frame chrome in any case

How does that sound?


FYI, the canonical documentation for the feature is still Eric Lawrence's blog post:
http://blogs.msdn.com/b/ieinternals/archive/2010/03/30/combating-clickjacking-with-x-frame-options.aspx
> My thinking about GetSameTypeRootTreeItem is:

It not relevant here, since in this case the chrome is being loaded in a content-type docshell, so the top content docshell in this case is the chrome page...  That old issue with chrome principal vs chrome docshell coming back to bite us.  :(
What are the chances of this making b6? I don't think it blocks it but it'd be super awesome to get it in as AMO are going to be rolling out the new UI in there next week
(In reply to comment #14)
> Comment on attachment 471988 [details] [diff] [review]
> fix v2

A new issue.

With current trunk and pending patch for bug 584693 applied, this patch no longer causes the Get Add-ons content to display.

For whatever it is worth, the original patch here at least still works.
The issue here seems to be that we are trying to display content in an iframe where the server specifically says:

x-frame-options: DENY

It seems to me this really should just not work.

Perhaps if the header said x-frame-options: SAMEORIGIN some exception for this case would make more sense?  Perhaps somehow treating about:addons and addons.mozilla.org as being the same origin? Still a kludge but makes more sense than permitting something that there is an explicit specific deny on the server-side.
Of course, if besides DENY and SAMEORIGIN, x-frame-options could also specify a whitelist of things permitted to include this in a frame, this would probably be helpful not only here, but in other cases as well.  But, I guess Microsoft never thought of that when they specified this.
Duplicate of this bug: 595494
(In reply to comment #19)
> What are the chances of this making b6? I don't think it blocks it but it'd be
> super awesome to get it in as AMO are going to be rolling out the new UI in
> there next week

This simple answer to the b6 issue would be to NOT specify the x-frame-options header on the 4.0b6 page on the server side, yet continue to specify it on the 4.0b7pre page so that this issue can continue to be worked.

This would result in Get Add-ons working for the beta builds only.
Depends on: 595634
(Assignee)

Comment 25

7 years ago
(In reply to comment #18)
> > My thinking about GetSameTypeRootTreeItem is:
> 
> It not relevant here, since in this case the chrome is being loaded in a
> content-type docshell, so the top content docshell in this case is the chrome
> page...  That old issue with chrome principal vs chrome docshell coming back to
> bite us.  :(

Bummer.  Well, that proposal was only to prevent the case in the first part of your comment 12:
> 1)  If a chrome page in a content docshell loads site A which then frames site
> B, the framing will be allowed with this patch.  Is this desired?

I suppose we can punt on that case since we ultimately get to decide who the site A is that we'll be framing in the product, and we can avoid choosing sites that will be framing other X-F-O:DENY pages.  Sound fair?
(Assignee)

Comment 26

7 years ago
And by "punt on that case" I mean "allow chrome to frame any document regardless of its X-F-O preferences".
It sounds like a footgun to me...  I doubt people expect that loading a chrome page in a content area will suddenly give it less secure behavior than loading it as chrome to start with.
(Assignee)

Comment 28

7 years ago
I am not sure if I am parsing your comment incorrectly or if you are misunderstanding the use case. Aren't we talking about loading a _content_ page in a _chrome area_?  This bug addresses the case where about:addons frames a page from services.addons.mozilla.org.

If we go forward with comment 26, the only cases where we end up with less secure behavior is if chrome frames a page that in turn frames another page who doesn't want to be framed.  This proposal would allow that inner page to be framed because the top-level context is chrome.  For there to be any real security impact, the page we chose to frame under the top-level chrome document would have to be a bad actor and do something malicious with the page it in turn framed (e.g. clickjacking).  It seems highly unlikely that we would frame any web content in the product that we don't trust or have control over.

Can you explain the danger you see here a bit more?  What are the cases where we load a chrome page in a content area and how is our comparison between the framed content and the top-level page not sufficient for those cases?
> Aren't we talking about loading a _content_ page in a _chrome area_?

We're talking about loading a document that has the system principal (so is trusted) in a type="content" docshell and this document then creating a subframe into which it loads an untrusted document.

If the document that has the system principal were loaded in a type="chrome" docshell itself, there would be no issue; X-Frame-Options would not treat it as the root document for the content and everything would Just Work (this is the case for the browser itself, say, where the XUL document for the UI is loaded in a chrome docshell and has type="content" subframes).  But the "ui in tab" thing is implemented in a particularly silly way in that it uses type="content" docshells.  As a result, anything that gets same-type docshell parents for subframes of that UI will find the docshell the UI is loaded in (e.g. if you frame AMO in the addons manager, window.top evaluated on AMO will actually return the addons manager window).  So X-Frame-Options is finding the addons UI as the root docshell, and you get this bug.

Are we on the same page so far?

> For there to be any real security impact, the page we chose to frame under
> the top-level chrome document would have to be a bad actor and do something
> malicious with the page it in turn framed

Yes.

> It seems highly unlikely that we would frame any web content in the product
> that we don't trust or have control over.

Where the first "we" includes extensions, right?  It doesn't seem that unlikely to me, given that...

> What are the cases where we load a chrome page in a content area

All the in-tab UI stuff, plus any extensions that do similar things.
(Assignee)

Comment 30

7 years ago
Just spoke to Boris and we decided on a better approach which is to mimic the GetSameTypeRootTreeItem behavior for finding the "top" docshell, but with an additional check that will make us stop when we find the first non-SystemPrinicpal-having item.  Patch coming up...
(Assignee)

Comment 31

7 years ago
Created attachment 475890 [details] [diff] [review]
fix v3

Find the top non-systemPrincipal docshell in our parent chain to avoid comparing principals with chrome documents (who are allowed to frame X-F-O pages).
Attachment #471988 - Attachment is obsolete: true
Attachment #475890 - Flags: review?(bzbarsky)
Comment on attachment 475890 [details] [diff] [review]
fix v3

>+    nsCOMPtr<nsIPrincipal> thisPrincipal = NodePrincipal();

Just nsIPrincipal* is fine.

>+                                  aDocShellItem = thisDocShellItem;

Call that curDocShellItem or something?

>+    nsCOMPtr<nsIDocument> topDoc;
>+    PRBool system = PR_FALSE;

You only use these inside the loop.  Why not just declare them there?

>+    nsIScriptSecurityManager *ssm = nsContentUtils::GetSecurityManager();
>+    if (!ssm)
>+      return NS_OK;

I'd think we want to fail safe here (so deny if we can't get an SSM).

>+    while (parentDocShellItem) {
>+      topDoc = do_GetInterface(parentDocShellItem);
>+      if (topDoc) {
>+        if (NS_SUCCEEDED(ssm->IsSystemPrincipal(topDoc->NodePrincipal(),
>+                                                &system)) && system) {

I'm pretty sure you should be able to assert that topDoc is non-null, but if we want ot keep the null-check we should just fail out if !topDoc. 

r=me woth those nits addressed.
Attachment #475890 - Flags: review?(bzbarsky) → review+
This is marked betaN but I would really like to see this in for feature freeze. Brandon, possible to get the comments and checkin turned around today?
(Assignee)

Comment 34

7 years ago
Created attachment 476053 [details] [diff] [review]
fix v4

This addresses Boris' review comments, but I'm still struggling to get a test working.  If drivers want to land this in the meantime, by all means...
Attachment #475890 - Attachment is obsolete: true
Attachment #476053 - Flags: review+
I really do not understand this bug at all.  THe client is trying to display from the server in a frame.  THe server sepcifacally says this content can not be displayed in a frame under any circumstances.  So exactly why should code be changed to actually allow the content to be displyed in the frame??
Because the "frame" in this case is actually the same thing as a browser tab for practical purposes.  It happens to have a slightly different implementation in the layout engine, but that's just a bug that needs to be fixed.  If that bug were fixed, this issue would not arise at all; this patch is just working around that bug.
(Assignee)

Comment 37

7 years ago
Created attachment 476434 [details] [diff] [review]
fix v5

Now with test.  Carrying forward Boris' review.
Attachment #476434 - Flags: review+
(Assignee)

Updated

7 years ago
Keywords: checkin-needed
If you're going to use checkin-needed, please include a commit message in your patch.  (Really, you should always include it so the reviewer can review it.)

I don't understand this well enough to write a good commit message, so I can't check it in.
http://hg.mozilla.org/mozilla-central/rev/f5c0015afe0e

I used the bug summary as the commit message, which seems suboptimal.
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b7
While we're at it, this bug seems to be in the wrong component as well.
Blocks: 598122

Updated

7 years ago
Duplicate of this bug: 594682

Updated

7 years ago
Duplicate of this bug: 601521
Verified fixed with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b7pre) Gecko/20101006 Firefox/4.0b7pre. Also tested with 4.0b8pre builds on other platforms.
Status: RESOLVED → VERIFIED
Component: Add-ons Manager → DOM
Flags: in-testsuite+
Flags: in-litmus-
Product: Toolkit → Core
QA Contact: add-ons.manager → general
Duplicate of this bug: 605005

Comment 45

7 years ago
(In reply to comment #43)
> Verified fixed with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b7pre)
> Gecko/20101006 Firefox/4.0b7pre. Also tested with 4.0b8pre builds on other
> platforms.

User-Agent:       Mozilla/5.0 (Windows NT 5.1; rv:2.0b6) Gecko/20100101
Firefox/4.0b6
Build Identifier: Mozilla/5.0 (Windows NT 5.1; rv:2.0b6) Gecko/20100101
Firefox/4.0b6

I'm here as a Dupe via Comment 44 .

Fixed ? : When I click [Tools][Add-ons] I get this page as the default http://www.mozilla.org/ , and if I click another Tab below "Get Add-ons" (EG: Extensions, Themes, or Plug-ins) and then "Get Add-ons" again I get a Blank rendering - there is NO frame. A right-click the SECOND time (in the frame area) says the URL is now "about:addons" (the "frame" is gone).


Rob
(Reporter)

Comment 46

7 years ago
(In reply to comment #45)
> (In reply to comment #43)
> > Verified fixed with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b7pre)
> > Gecko/20101006 Firefox/4.0b7pre. Also tested with 4.0b8pre builds on other
> > platforms.
> 
> User-Agent:       Mozilla/5.0 (Windows NT 5.1; rv:2.0b6) Gecko/20100101
> Firefox/4.0b6
> Build Identifier: Mozilla/5.0 (Windows NT 5.1; rv:2.0b6) Gecko/20100101
> Firefox/4.0b6
> 
> I'm here as a Dupe via Comment 44 .
> 
> Fixed ? : When I click [Tools][Add-ons] I get this page as the default
> http://www.mozilla.org/ , and if I click another Tab below "Get Add-ons" (EG:
> Extensions, Themes, or Plug-ins) and then "Get Add-ons" again I get a Blank
> rendering - there is NO frame. A right-click the SECOND time (in the frame
> area) says the URL is now "about:addons" (the "frame" is gone).
> 
> 
> Rob

It's been fixed after beta 6 was released. The (hopefully) upcoming beta 7 will include the fix.
You need to log in before you can comment on or make changes to this bug.