gecko should not attempt to download frame content

VERIFIED FIXED in mozilla0.9

Status

()

Core
Embedding: APIs
--
major
VERIFIED FIXED
17 years ago
16 years ago

People

(Reporter: Marek Z. Jeziorek, Assigned: harishd)

Tracking

(Blocks: 1 bug, {embed})

Trunk
mozilla0.9
embed
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(9 attachments)

(Reporter)

Description

17 years ago
Some customers do not allow frames in mail, so gecko should not attempt to 
download frame content their behalf.
(Reporter)

Updated

17 years ago
Blocks: 64833

Comment 1

17 years ago
-> rpotts. Rick, where should this land?
Assignee: valeski → rpotts

Comment 2

17 years ago
-> Eric
Assignee: rpotts → pollmann

Updated

17 years ago
No longer blocks: 64833

Comment 3

17 years ago
This should be a setting on nsIWebBrowserSetup.
Blocks: 64833
Keywords: mozilla0.9

Updated

17 years ago
Blocks: 70229
(Reporter)

Comment 4

17 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

17 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

17 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.
(Assignee)

Comment 7

17 years ago
3) would require parser/sink work but it shouldn't be too difficult.

Comment 8

17 years ago
Created attachment 29166 [details] [diff] [review]
api changes to webbrowsersetup and docshell

Comment 9

17 years ago
Handing to Harish to come up with the second half of the fix.
Assignee: pollmann → harishd
Status: ASSIGNED → NEW

Comment 10

17 years ago
Created attachment 29780 [details] [diff] [review]
Patch: Turn off framesets/iframes, display remaining content, allow scrollbars if allowSubframes turned off.

Comment 11

17 years ago
Created attachment 29781 [details] [diff] [review]
Update of patch 2: QI is null safe, removing extra checks :)

Comment 12

17 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?

Updated

17 years ago
Blocks: 56743
(Assignee)

Comment 13

17 years ago
Created attachment 29931 [details] [diff] [review]
Patch v1.1 [ Enable noframes content when frames are disabled. Note: includes Eric's docshell changes ]

Comment 14

17 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

17 years ago
Created attachment 29984 [details] [diff] [review]
Merged patch

Comment 16

17 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

17 years ago
Created attachment 30336 [details] [diff] [review]
patch, simplified changes to docshell and all.js

Comment 18

17 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

17 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

17 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

17 years ago
Second example in 3) above should read:

<html>
<frameset> </frameset>
<noframes> <body> <p> Lamer! </p> </body> </noframes>
</html>

(Assignee)

Comment 22

17 years ago
Created attachment 30780 [details] [diff] [review]
Patch v1.5 [ includes htmlparser,content,docshell,embedding,modules ]

Comment 23

17 years ago
Created attachment 30984 [details] [diff] [review]
Patch v1.6 [ includes layout,htmlparser,content,docshell,embedding,modules ]
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

17 years ago
rs=waterson

Comment 26

17 years ago
Correction: Changing QA contact for the Embed API bugs to David Epstein.
QA Contact: mdunn → depstein
(Assignee)

Comment 27

17 years ago
Created attachment 31339 [details] [diff] [review]
Patch v1.7

Comment 28

17 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

17 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
Typo?

+#ifdef NS_DEBUH
         if(mDTDDebug) { 
           mDTDDebug->DumpVectorRecord(); 
         } 
+#endif

(Assignee)

Comment 31

17 years ago
yup, that's a typo. Corrected.
(Assignee)

Comment 32

17 years ago
Fix is in. Marking FIXED.
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED

Comment 33

17 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

17 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
(Reporter)

Updated

16 years ago
No longer blocks: 64833
You need to log in before you can comment on or make changes to this bug.