Closed Bug 69455 Opened 24 years ago Closed 23 years ago

gecko should not attempt to download frame content

Categories

(Core Graveyard :: Embedding: APIs, defect)

Product:
Core Graveyard
Component:
Embedding: APIs
Type:
defect
Priority:
Not set
Severity:
major

Tracking

(Not tracked)

Status:
VERIFIED FIXED
Milestone:
mozilla0.9

People

(Reporter: bugz, Assigned: harishd)

References

Details

(Keywords: embed)

Attachments

(9 files)

api changes to webbrowsersetup and docshell
3.85 KB, patch
Details | Diff | Splinter Review
Patch: Turn off framesets/iframes, display remaining content, allow scrollbars if allowSubframes turned off.
4.75 KB, patch
Details | Diff | Splinter Review
Update of patch 2: QI is null safe, removing extra checks :)
4.58 KB, patch
Details | Diff | Splinter Review
Patch v1.1 [ Enable noframes content when frames are disabled. Note: includes Eric's docshell changes ]
22.85 KB, patch
Details | Diff | Splinter Review
Merged patch
25.47 KB, patch
Details | Diff | Splinter Review
patch, simplified changes to docshell and all.js
1.90 KB, patch
Details | Diff | Splinter Review
Patch v1.5 [ includes htmlparser,content,docshell,embedding,modules ]
34.76 KB, patch
Details | Diff | Splinter Review
Patch v1.6 [ includes layout,htmlparser,content,docshell,embedding,modules ]
41.23 KB, patch
Details | Diff | Splinter Review
Patch v1.7
46.87 KB, patch
Details | Diff | Splinter Review 
Some customers do not allow frames in mail, so gecko should not attempt to 
download frame content their behalf.
Blocks: 64833
-> rpotts. Rick, where should this land?
Assignee: valeski → rpotts
-> Eric
Assignee: rpotts → pollmann
No longer blocks: 64833
This should be a setting on nsIWebBrowserSetup.
Blocks: 64833
Keywords: mozilla0.9
Blocks: 70229
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.
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
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.
Handing to Harish to come up with the second half of the fix.
Assignee: pollmann → harishd
Status: ASSIGNED → NEW
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?
Blocks: frame-off
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.
Attached patch Merged patch — — Splinter Review 

    
    

    
  
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?


    
    

    
  
This ammendment removes the negative logic from the pref, which probably makes
things easier to understand, and removes the local PRBool cruft...


    
    

    
  
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...)

    
    

    
  
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.



    
    

    
  
Second example in 3) above should read:

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



    
    

    
  
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.

    
    

    
  
rs=waterson

    
    

    
  
Correction: Changing QA contact for the Embed API bugs to David Epstein.
QA Contact: mdunn → depstein

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch v1.7
        — 
          — Splinter Review
      

    


  

    
    

    
  
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

    
    

    
  
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



    
    

    
  
yup, that's a typo. Corrected.

    
    

    
  
Fix is in. Marking FIXED.
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED

    
    

    
  
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.


    
    

    
  
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

    
  
No longer blocks: 64833

    
  
Product: Core → Core Graveyard

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: