Closed
Bug 423833
Opened 17 years ago
Closed 17 years ago
Show Only This Frame code uses about: url for error pages, instead of original site url
Categories
(Firefox :: Menus, defect, P2)
Tracking
()
VERIFIED
FIXED
Firefox 3
People
(Reporter: jo.hermans, Assigned: johnath)
References
Details
(Keywords: regression)
Attachments
(1 file, 4 obsolete files)
(deleted),
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 ?
Assignee | ||
Comment 1•17 years ago
|
||
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
Assignee | ||
Updated•17 years ago
|
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
Assignee | ||
Comment 2•17 years ago
|
||
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
Assignee | ||
Comment 3•17 years ago
|
||
Not tagging for review until I have some tests
Assignee: nobody → johnath
Status: NEW → ASSIGNED
Assignee | ||
Comment 4•17 years ago
|
||
Two more instances of the same issue.
Attachment #310469 -
Attachment is obsolete: true
Comment 5•17 years ago
|
||
Neil, Seamonkey might need similar changes.
Updated•17 years ago
|
Flags: blocking-firefox3? → blocking-firefox3+
Priority: -- → P2
Comment 6•17 years ago
|
||
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.
Assignee | ||
Comment 7•17 years ago
|
||
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)
Comment 8•17 years ago
|
||
In the tests, you could use the DOMContentLoaded event.
Comment 9•17 years ago
|
||
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.
Updated•17 years ago
|
Attachment #310779 -
Flags: review?(mano) → review-
Updated•17 years ago
|
Whiteboard: [needs status update]
Assignee | ||
Comment 10•17 years ago
|
||
(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)
Assignee | ||
Updated•17 years ago
|
Whiteboard: [needs status update] → [has patch][needs review mano]
Comment 11•17 years ago
|
||
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+
Assignee | ||
Updated•17 years ago
|
Whiteboard: [has patch][needs review mano]
Assignee | ||
Comment 12•17 years ago
|
||
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: 17 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 13•17 years ago
|
||
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 → ---
Assignee | ||
Comment 14•17 years ago
|
||
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
Assignee | ||
Comment 15•17 years ago
|
||
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 16•17 years ago
|
||
Comment on attachment 314105 [details] [diff] [review]
Better load handling in tests
r=mano
Attachment #314105 -
Flags: review?(mano) → review+
Assignee | ||
Comment 17•17 years ago
|
||
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
Comment 18•17 years ago
|
||
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: 17 years ago → 17 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3
Comment 19•17 years ago
|
||
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
Comment 20•17 years ago
|
||
Ah, looks like that part of the checkin was actually part of bug 295994, and was mislabeled.
Reporter | ||
Comment 21•17 years ago
|
||
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.
Assignee | ||
Comment 22•17 years ago
|
||
(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?
Reporter | ||
Comment 23•17 years ago
|
||
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
Comment 24•17 years ago
|
||
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.
Comment 26•17 years ago
|
||
(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.
Description
•