Closed
Bug 69455
Opened 24 years ago
Closed 24 years ago
gecko should not attempt to download frame content
Categories
(Core Graveyard :: Embedding: APIs, defect)
Core Graveyard
Embedding: APIs
Tracking
(Not tracked)
VERIFIED
FIXED
mozilla0.9
People
(Reporter: bugz, Assigned: harishd)
References
Details
(Keywords: embed)
Attachments
(9 files)
|
(deleted),
patch
|
Details | Diff | Splinter Review | |
|
(deleted),
patch
|
Details | Diff | Splinter Review | |
|
(deleted),
patch
|
Details | Diff | Splinter Review | |
|
(deleted),
patch
|
Details | Diff | Splinter Review | |
|
(deleted),
patch
|
Details | Diff | Splinter Review | |
|
(deleted),
patch
|
Details | Diff | Splinter Review | |
|
(deleted),
patch
|
Details | Diff | Splinter Review | |
|
(deleted),
patch
|
Details | Diff | Splinter Review | |
|
(deleted),
patch
|
Details | Diff | Splinter Review |
Some customers do not allow frames in mail, so gecko should not attempt to
download frame content their behalf.
|
||
Comment 3•24 years ago
|
||
This should be a setting on nsIWebBrowserSetup.
Blocks: 64833
Keywords: mozilla0.9
|
Reporter | |
Comment 4•24 years ago
|
||
Eric, this bug is necessary for our embedding customers. Could you get it done
either in 0.8.1 or 0.9 milestone (and set the target mielstone accordingly).
Thanks.
|
||
Comment 5•24 years ago
|
||
Disabling frames is going to take quite a bit of work. I'll try for 0.9.
CC'ing Harish as he'll also probably need to do some work for this fix.
Severity: normal → major
Status: NEW → ASSIGNED
Keywords: embed
OS: Windows 98 → All
Hardware: PC → All
Target Milestone: --- → mozilla0.9
|
|
||
Comment 6•24 years ago
|
||
Thoughts on what should be displayed in place of the frameset?
1) blank page
2) explanatory text
3) <noframes> content
These are probably close to ordered from simplest to most difficult. 1) would
be trivial, 2) would be somewhat easy, 3) would be harder and would probably
involve changes in the parser and/or content sink.
3) would require parser/sink work but it shouldn't be too difficult.
|
|
||
Comment 8•24 years ago
|
||
|
|
||
Comment 9•24 years ago
|
||
Handing to Harish to come up with the second half of the fix.
Assignee: pollmann → harishd
Status: ASSIGNED → NEW
|
|
||
Comment 10•24 years ago
|
||
|
|
||
Comment 11•24 years ago
|
||
|
|
||
Comment 12•24 years ago
|
||
Harish, the patches I've attached above can be checked in independently of the
patches to enable <noframes> content - should I go ahead and do that?
|
Assignee | |
Comment 13•24 years ago
|
||
|
|
||
Comment 14•24 years ago
|
||
Harish, looking at OpenAlternateContent, I just noticed GetAllowJavascript
got recently added to nsDocShell.cpp:
http://lxr.mozilla.org/seamonkey/source/docshell/base/nsDocShell.cpp#705
It seems to me that you would also want to enable <noscript> if this was set to
PR_FALSE?
We could move the code that checks the pref into nsDocShell's constructor to
create a default value for mAllowJavascript, and do the same thing for a pref
for mAllowSubframes. :) This way, the pref would only get checked once instead
of at every <noscript> and <noframes> tag, and could be over-ridden by calls to
the APIs on nsIWebBrowserSetup.
|
|
||
Comment 15•24 years ago
|
||
|
|
||
Comment 16•24 years ago
|
||
Harish, I think this Merged patch combines all of our changes into one. I also
took the liberty of implementing that last change. Now frames are controllable
via a pref with no UI! :) Also had to make a parallel Script->AlternateContent
name change in nsRobotSink.cpp to get the beast to compile - is that okay?
|
|
||
Comment 17•24 years ago
|
||
|
|
||
Comment 18•24 years ago
|
||
This ammendment removes the negative logic from the pref, which probably makes
things easier to understand, and removes the local PRBool cruft...
|
||
Comment 19•24 years ago
|
||
Couple comments:
1. In theory, couldn't someone still send an XML document that contains
an <html:frame>?
2. It's not clear to me that we do the right thing if there isn't a <NOFRAMES>
tag. Do we still perform the initial reflow? (Or are we left in a state like
bug 5569?)
3. If there is a <NOFRAMES> tag, what does the content model end up looking
like? I'd guess it'd be something like...
<FRAMESET>
<NOFRAMES>
<P>Lamer!</P>
</NOFRAMES>
</FRAMESET>
I seem to recall code in the nsHTMLDocument that expects to be able to find
a <BODY> tag and stuff; e.g., GetBodyElement(). Will this stuff fail-safe?
(It looks like it should from a quick glance at it...)
4. I assume that an embedder can do one-off disabling of framesets in a
docshell by calling nsIDocShell::SetAllowSubframes()? (If that's the case,
is the pref really necessary? Who'd want to turn off framesets by a pref?
Doesn't hurt, but...)
|
|
||
Comment 20•24 years ago
|
||
1. Yes, but I think we would still be okay here. Unless the html:frame had an
html:frameset as a parent (in which case we would also be okay), it would not
actually result in a nsHTMLFrameOuterFrame. This is because html frames are
only constructed by nsHTMLFramesetFrame. nsCSSFrameConstructor only knows about
html framesets, html iframes, and xul iframes. It does not know about html
frames (Pretty bad, I know, but that's another issue. :) )
2. Good call, in fact, that is exactly what happens if frames are turned off and
we have:
<html>
<frameset>
<frame src="about:blank">
</frameset>
</html>
The window just doesn't paint. Actually, this seems more dependent on whether
there is a <body> anywhere in the document. If not, we don't paint. It doesn't
seem to care if there is a <noframes> around the <body> or not. We definitely
need to solve this problem still!
3. Well, I just tried in viewer. When we have:
<html>
<frameset>
<noframes> <body> <p> Lamer! </p> </body> </noframes>
</frameset>
</html>
The content model ends up like:
<frameset></frameset>
This is bad. Harish, we need to handle this case differently from the <script>
/ </noscript> case somehow...
On the other hand, when we have:
<html>
<frameset>
<noframes> <body> <p> Lamer! </p> </body> </noframes>
</frameset>
</html>
The content model ends up like:
<html>
<frameset> </frameset>
<body> <p> Lamer! </p> </body>
</html>
The <noframe> is stripped out. Harish, this is as designed, right?
4. The pref is going to be needed for bug 56743.
|
|
||
Comment 21•24 years ago
|
||
Second example in 3) above should read:
<html>
<frameset> </frameset>
<noframes> <body> <p> Lamer! </p> </body> </noframes>
</html>
|
|
Assignee | |
Comment 22•24 years ago
|
||
|
|
||
Comment 23•24 years ago
|
||
These are mostly for the parts harishd has created. I need to check with
pollmann for him to explain me his stuff.
1) Docshell has a lot of PRBool members. Maybe it would be time to make them
into flags, as in parser? Saves a little space, but maybe it is insignificant.
2) Is docshell interface frozen or not? Please check the embedding pages for
lists of interfaces that are considered frozen.
3) Order the contructor initializer list so that it matches the order in which
the members appear in the class declaration (otherwise gcc et. al. will warn -
check with gcc).
4) I question SetCapacity(0). I think you should not do that. As far as I
understand, it will cause us to release the string's internal buffer, so next
time someone uses the string we have to reallocate the internal buffer. Better
to reuse the buffer we had, if it is big enough. This is not a big memory issue,
especially since the parser object will go away as soon as it has parsed the data.
5) Please use parenthesis to explicitly group stuff in the big if-clause around
line 541. Some weird compilers might get this wrong. You also mix two kinds of
notations in one file: ~mFlags... and !(mFlags...), it would be better to stick
with one, I think. Also I think the b) claim in the comment does not match what
you are doing in the if-clause: the if says (to me at least): did not have
frameset and did not have frames enabled.
6) Add the bug number in stead of XXXXXX.
7) The start and stop timer seems a bit weird to me. I would think that we would
stop timer after we are done, not stop if if we are NOT done. Please check.
8) Please check whitespace, it appears there are some unwanted tabs.
|
|
||
Comment 25•24 years ago
|
||
rs=waterson
|
||
Comment 26•24 years ago
|
||
Correction: Changing QA contact for the Embed API bugs to David Epstein.
QA Contact: mdunn → depstein
|
|
Assignee | |
Comment 27•24 years ago
|
||
|
|
||
Comment 28•24 years ago
|
||
Why's this code commented out?
+#ifdef NS_DEBUG
if (mDTDDebug) {
- mDTDDebug->Verify(this, mParser, mBodyContext->GetCount(),
mBodyContext->mStack, mFilename);
+ //mDTDDebug->Verify(this, mParser, mBodyContext->GetCount(),
mBodyContext->mStack, mFilename);
}
#endif
sr=waterson
|
|
Assignee | |
Comment 29•24 years ago
|
||
The commented out code is not in use. I should either change the verify() method
or should remove it altogether. Will work on it when I have time.
Status: NEW → ASSIGNED
|
||
Comment 30•24 years ago
|
||
Typo?
+#ifdef NS_DEBUH
if(mDTDDebug) {
mDTDDebug->DumpVectorRecord();
}
+#endif
|
|
Assignee | |
Comment 31•24 years ago
|
||
yup, that's a typo. Corrected.
|
|
Assignee | |
Comment 32•24 years ago
|
||
Fix is in. Marking FIXED.
Status: ASSIGNED → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
|
||
Comment 33•24 years ago
|
||
checked some of source file changes against patch checkins:
1)4/18/01 checkin:
* In nsRobotSink.cpp: OpenNoscript() and CloseNoscript() were removed. Added
GetPref(PRInt32 aTag,PRBool& aPref) { return NS_OK; }
* In CNavDTD.cpp, changes made to #define NS_PARSER_FLAG_*. Additions & changes
made to CNavDTD::CNavDTD() : nsIDTD(). e.g. mOpenHeadCount=0 changed to
mOpenHeadCount(0), same for other indicators. if(mSink){} snipped added.
if(!(mFlags & NS_PARSER_FLAG_FRAMES_ENABLED) {} snippet added. case
eHTMLTag_noscript code added. Code removed from case eHTMLTag_noscript, code
added to case eHTMLTag_noframes.
* In CNavDTD.h, removed OpenNoscript() and CloseNoscript() declarations. Added
indicators like mMisplacedContent, mSkippedContent, etc. Removed counter like
mAlternateTagOpenCount. Moved mLineNumber.
* In nsElementTable.cpp, changes made to Initialize(): parents/kids tags.
more verification later.
|
|
||
Comment 34•24 years ago
|
||
I had made additional verifications to the 4/18/01 checkin, and entered comments
here, but bugzilla crashed for me before I could submit. :( . So rather than
re-enter every single verification, I'll just give the hilites:
* Removed Open/CloseNoScript() and added GetPref() to:
nsHTMLContentSinkStream.h, nsHTMLNullSink.cpp, nsIHTMLContentSink.h,
nsLoggingSink.h, nsPlainTextSerializer.cpp, nsPlainTextSerializer.h, &
nsHTMLContentSink.cpp.
* In nsWebBrowser.cpp, added: case nsIWebBrowserSetup::SETUP_ALLOW_SUBFRAMES
* In all.js, added: pref("browser.frames.enabled", true);
* In nsIDocShell.idl, added attribute allowSubframes.
* In nsDocShell.cpp, added Get/SetAllowSubframes().
* In nsCSSFrameConstructor.cpp, added allowSubframes support.
* In html.css, removed noframes from Hidden Elements, added noframes leaf.
* In nsHTMLFragmentContentSink.cpp, added GetPref().
Status: RESOLVED → VERIFIED
|
||
Updated•6 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•