Closed Bug 423833 Opened 16 years ago Closed 16 years ago

Show Only This Frame code uses about: url for error pages, instead of original site url

Categories

(Firefox :: Menus, defect, P2)

Product:
Firefox
Component:
Menus
Platform:
x86
Windows XP
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 3

People

(Reporter: jo.hermans, Assigned: johnath)

References

Details

(Keywords: regression)

Attachments

(1 file, 4 obsolete files)

Better load handling in tests
7.54 KB, patch
asaf
: review+
Details | Diff | Splinter Review 
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b5pre) Gecko/2008031806 Minefield/3.0b5pre

After bug 423247 (Certificate error for iframe is shown as a dialog instead of an error page, making it impossible to add an exception) was checked in, I wanted to check the new behavior on a page that used to suffer from this problem.

The original URL is on the intranet of my company, but it contained a frame at the top for which no valid security certificate was present. Because the error was displayed in a dialog box, you couldn't easily add the certificate to the exceptions list, unless you copied the URL form the dialog box, and manually went to the advanced options.

After bug 423247 was checked in (even though that mentions an iframe in the title, but this was about a frame), the normal error page was show instead of the dialog box. You can check the result here : <http://img301.imageshack.us/my.php?image=screenshotyq5.png>

Since no scrollbars are present (due to the way the frame was build), I couldn't read the 'add an exception link'. So I wanted to be clever, and I selected 'show only this frame' in the context menu. Then I saw the normal error page, as you can see here : <http://img329.imageshack.us/my.php?image=screenshotqb4.png>. Note the URL, which is <about:neterror?e=nssBadCert&u=https%3A//all.alcatel-lucent.com/wps/portal/iframe%3Flu_lang_code%3Den&c=ISO-8859-1&d=all.alcatel-lucent.com%20uses%20an%20invalid%20security%20certificate.%0A%0AThe%20certificate%20is%20not%20trusted%20because%20the%20issuer%20certificate%20is%20not%20trusted.%0A%0A(Error%20code%3A%20sec_error_untrusted_issuer)%0A>.

Then I clicked on the 'Or you can add an exception...' link, but that didn't work. I got into the 'Add Security Exception' dialog box (impossible to take a screendump on a modal dialog, sorry), but clicking on 'Get Certificate' only proceed a 'Attempting to identify the site' warning. The reason is that the URL is the about:neterror URL, and not the one that caused the error.

Maybe should 'Show only this frame' try to show the frame, and not the 'about:neterror' page ? Or the about:neterror page should try to open the dialog box on the original URL ?
about:neterror should be invisible to users at all times, so this is a bug in the "show only this frame" code, I'd say.

Verifiable by using this URL, which is free of any certificates.

data:text/html,<html><frameset cols="500,500"><frame src="http://google.com"><frame src="http://foo.foo"></frameset></html>

Obviously it's exacerbated in the case where that page has useful things to interact with, though.

The offending code appears to be here: http://mxr.mozilla.org/mozilla/source/browser/base/content/nsContextMenu.js#680

Moving over to Firefox
Assignee: kengert → nobody
Component: Security: PSM → Menus
Product: Core → Firefox
QA Contact: psm → menus
Summary: trying to add an certificate-exception from an about:neterror page → Show Only This Frame code uses about: url for error pages, instead of original site url
Nominating for blocking.  This is a regression from firefox 2 behaviour, apparently from bug 376189.  I don't think it needs beta exposure, so P2 would be fine, but we depend on the error pages more now (ssl errors, malware) so it's likely going to occur more often that users "show only this frame" to get better access to that content.
Flags: blocking-firefox3?
Keywords: regression
Not tagging for review until I have some tests
Assignee: nobody → johnath
Status: NEW → ASSIGNED
Blocks: 376189
Attached patch Catch other instances (obsolete) — — Splinter Review 
Two more instances of the same issue.
Attachment #310469 - Attachment is obsolete: true
Neil, Seamonkey might need similar changes.
Flags: blocking-firefox3? → blocking-firefox3+
Priority: -- → P2
Well, we only have ten hits:
tabbrowser.xml: used for a content policy check for about:neterror's favicon
navigator.js: used to determine the page that loaded a favicon
permissions.js: apparently used to determine whether a page is secure
notification.xml: used to generate an absolute URL for the plugin finder
utilityOverlay.js: used to detect about:neterror for security error buttons
The other hits are used to provide a referrer e.g. for saving links.
Attached patch With tests (obsolete) — — Splinter Review 
Patch looks bigger than it is - only 3 lines changed, the rest is tests.
Attachment #310470 - Attachment is obsolete: true
Attachment #310779 - Flags: review?(mano)
In the tests, you could use the DOMContentLoaded event.
Also, openFrame and openFrameInTab should return the newly created tab or window, then you could get rid of the windowwatcher usage. I could see potential use-cases for this outside of the testsuite.
Attachment #310779 - Flags: review?(mano) → review-
Whiteboard: [needs status update]
(In reply to comment #9)
> Also, openFrame and openFrameInTab should return the newly created tab or
> window, then you could get rid of the windowwatcher usage. I could see
> potential use-cases for this outside of the testsuite.

Interesting, and I agree, potentially useful.  I have done this, which obviously also required modifying utilityOverlay to return from the underlying calls (why didn't they?) 

(In reply to comment #8)
> In the tests, you could use the DOMContentLoaded event.

DOMContentLoaded got me into a race situation - re-running the same tests would sometimes get failures where new tab or window was opened with blank instead of the page url or error page.  I have switched setTimeout to setInterval though, as a way to prevent spurious timeout failures.
Attachment #310779 - Attachment is obsolete: true
Attachment #312964 - Flags: review?(mano)
Whiteboard: [needs status update] → [has patch][needs review mano]
Comment on attachment 312964 [details] [diff] [review]
Modify openFrame, openFrameInTab to return, + tests to make sure they return sane things

ok, r=mano.
Attachment #312964 - Flags: review?(mano) → review+
Whiteboard: [has patch][needs review mano]
Checking in browser/base/content/nsContextMenu.js;
/cvsroot/mozilla/browser/base/content/nsContextMenu.js,v  <--  nsContextMenu.js
new revision: 1.56; previous revision: 1.55
done
Checking in browser/base/content/utilityOverlay.js;
/cvsroot/mozilla/browser/base/content/utilityOverlay.js,v  <--  utilityOverlay.js
new revision: 1.66; previous revision: 1.65
done
Checking in browser/base/content/test/Makefile.in;
/cvsroot/mozilla/browser/base/content/test/Makefile.in,v  <--  Makefile.in
new revision: 1.10; previous revision: 1.9
done
RCS file: /cvsroot/mozilla/browser/base/content/test/browser_bug423833.js,v
done
Checking in browser/base/content/test/browser_bug423833.js;
/cvsroot/mozilla/browser/base/content/test/browser_bug423833.js,v  <--  browser_bug423833.js
initial revision: 1.1
done
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Backing out due to windows test bustage, though mac worked.  :(

Log: http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1207576833.1207579665.21121.gz#err0

Checking in browser/base/content/nsContextMenu.js;
/cvsroot/mozilla/browser/base/content/nsContextMenu.js,v  <--  nsContextMenu.js
new revision: 1.57; previous revision: 1.56
done
Checking in browser/base/content/utilityOverlay.js;
/cvsroot/mozilla/browser/base/content/utilityOverlay.js,v  <--  utilityOverlay.js
new revision: 1.67; previous revision: 1.66
done
Checking in browser/base/content/test/Makefile.in;
/cvsroot/mozilla/browser/base/content/test/Makefile.in,v  <--  Makefile.in
new revision: 1.11; previous revision: 1.10
done
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
The old tests anticipated that there would be 2 load events when loading the (2-frame) page, and had a safeguard in place for that, which was enough to pass for me.  But there was a race there, and depending on the order of things, it could happen that the context-menu calls would happen before the error page had loaded.  This would cause the new tab or window to be opened blank, instead of with the error page.

Rather than playing guessing games with the number of loads triggered, this patch just checks the document.location of the error page frame, and doesn't do anything until it it set properly.  Will ask for review once I've done tests on mac (already passed) *and* vista.
Attachment #312964 - Attachment is obsolete: true
Comment on attachment 314105 [details] [diff] [review]
Better load handling in tests

Gavin has run this against his Vista build and it passed.  Knocking to Mano for review
Attachment #314105 - Flags: review?(mano)
Comment on attachment 314105 [details] [diff] [review]
Better load handling in tests

r=mano
Attachment #314105 - Flags: review?(mano) → review+
Thanks Mano.  Marking checkin-needed, because I'm travelling over the next few days, and may not have a contiguous block of time free to land and watch the tree.  If someone gets to this bug before I do, please feel free and accept my appreciation.  :)
Keywords: checkin-needed
Checking in browser/base/content/nsContextMenu.js;
/cvsroot/mozilla/browser/base/content/nsContextMenu.js,v  <--  nsContextMenu.js
new revision: 1.58; previous revision: 1.57
done
Checking in browser/base/content/utilityOverlay.js;
/cvsroot/mozilla/browser/base/content/utilityOverlay.js,v  <--  utilityOverlay.js
new revision: 1.68; previous revision: 1.67
done
Checking in browser/base/content/test/Makefile.in;
/cvsroot/mozilla/browser/base/content/test/Makefile.in,v  <--  Makefile.in
new revision: 1.12; previous revision: 1.11
done
Checking in browser/base/content/test/browser_bug423833.js;
/cvsroot/mozilla/browser/base/content/test/browser_bug423833.js,v  <--  browser_bug423833.js
new revision: 1.2; previous revision: 1.1
done
Status: REOPENED → RESOLVED
Closed: 16 years ago → 16 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3
test_bug398289.html is currently failing on Linux, which Bonsai thinks was modified as part of this patch.

* Bonsai page: http://tinyurl.com/4zlc72
* File itself: http://bonsai.mozilla.org//cvsblame.cgi?file=mozilla/content/xul/content/test/test_bug398289.html
* Failure Log: http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1207682613.1207686773.29781.gz

Ah, looks like that part of the checkin was actually part of bug 295994, and was mislabeled.
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008040907 Minefield/3.0pre

I verified the fix on the location mentioned in comment 0, but since this was a private intranet site, I can't close the bug yet.
(In reply to comment #21)
> Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008040907
> Minefield/3.0pre
> 
> I verified the fix on the location mentioned in comment 0, but since this was a
> private intranet site, I can't close the bug yet.

Jo - my data url in comment 1 should work for public verification, shouldn't it? 


Yes, but I thought you had verified that yourself. My original URL was one of those bad-certificate errors, where you couldn't add an exception anymore.
Status: RESOLVED → VERIFIED
I checked in a change to the test to reduce the polling interval to 1 second from 3, to let the tests potentially finish quicker if possible.
(In reply to comment #24)
> I checked in a change to the test to reduce the polling interval to 1 second
> from 3, to let the tests potentially finish quicker if possible.

...then I backed it out because it was causing the tests to fail on Windows.

I just disabled the test because it's been timing out on the linux machine intermittently since it landed, and increasing the global browser test timeout to 30s (from 15s) didn't help. I filed bug 428712.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: