Closed Bug 873864 Opened 11 years ago Closed 11 years ago

Test failures when reloading test_bug669671.html

Categories

(Core :: DOM: Navigation, defect)

Product:
Core
Component:
DOM: Navigation
Platform:
x86
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla25

People

(Reporter: martijn.martijn, Assigned: martijn.martijn)

Details

Attachments

(1 file, 3 obsolete files)

test_bug669671.diff for check-in
1.76 KB, patch
Details | Diff | Splinter Review
Attached patch patch (obsolete) — — Splinter Review 
I was looking at bug 858055 and when I loaded this test standalone, I noticed I get failures when reloading it.
This patch fixes it, but I'm wondering about the todo part of the patch I had to add. That seems like a bug to me.
When this code is reached, "popup.location = 'file_bug669671.sjs?countreset';", it seems that Firefox is fetching the page from cache, when it should fetch an uncached page, no?
Attachment #751496 - Flags: feedback?(justin.lebar+bug)
> When this code is reached, "popup.location = 'file_bug669671.sjs?countreset';", it seems 
> that Firefox is fetching the page from cache, when it should fetch an uncached page, no?

If you've loaded foo.html, Firefox cannot serve foo.html?bar from cache.  Those are two completely different resources.

So if Firefox is fetching file_bug669671.html?countreset from cache, that would be super-bad...

Is the problem just that we don't reset the |count| variable in the sjs file after we setState('count', 0)?
Attachment #751496 - Flags: feedback?(justin.lebar+bug)
(In reply to Justin Lebar [:jlebar] from comment #1)
> So if Firefox is fetching file_bug669671.html?countreset from cache, that would be 
> super-bad...

That's the only thing I can explain the todo() thing that I had to add in this patch, but perhaps I did something wrong in the patch.

> Is the problem just that we don't reset the |count| variable in the sjs file
> after we setState('count', 0)?

Do you think that is happening here? You think there is a bug in the setState function of httpd.js?
I'm using setState('count', '0'), btw, not setState('count', 0), because only strings are allowed.
>    var count = parseInt(getState('count'));
>    if (!count)
>      count = 0;
>    setState('count', count + 1 + '');
>  
> +  if (request.queryString == 'countreset')
> +    setState('count', '0');
> +
>    response.setHeader('Content-Type', 'text/html', false);
>    response.setHeader('Cache-Control', 'max-age=0');
> -  response.write('<html><body onload="opener.onChildLoad()" ' +
> +  response.write('<html><body onload="opener.onChildLoad();" ' +
>                   'onunload="parseInt(\'0\')">' +
>                   count + '</body></html>');

I was trying to say that I think this patch has a bug.  We should have |count = 0| inside the |if|that contains |setState('count', '0');|.  |setState| isn't supposed to modify the value of the local variable |count|.
Or you could move the setState call to above the getState call.  :shrug:
Ah, duh! Thanks!
Attached patch patch (obsolete) — — Splinter Review 

        Attachment #751496 -
        Attachment is obsolete: true

        Attachment #762417 -
        Flags: review?(justin.lebar+bug)

    
  
Assignee: nobody → martijn.martijn

    
    

    
  
Comment on attachment 762417 [details] [diff] [review]
patch

Maybe we should reset the count at the beginning of the test.  That way, if the test hangs for whatever reason, you can refresh it and it won't give the wrong result.

Also, please add a comment explaining why we bother to countreset.

r=me with those nits.

        Attachment #762417 -
        Flags: review?(justin.lebar+bug) → review+

    
    

    
  

      
      
      
      

        Attached patch
          
          
        test_bug669671.diff (obsolete)
        — 
          — Splinter Review
      

    


  
I think this is what you want then. 
Although this seems to make step 1 and the comment in step 1 rather moot. I guess that step can be removed (and then make step 0, step 1).

        Attachment #762417 -
        Attachment is obsolete: true

        Attachment #780705 -
        Flags: review?(justin.lebar+bug)

    
    

    
  
Comment on attachment 780705 [details] [diff] [review]
test_bug669671.diff

r=me

        Attachment #780705 -
        Flags: review?(justin.lebar+bug) → review+

    
    

    
  


  
For check-in.

    
  
Keywords: checkin-needed

        Attachment #780705 -
        Attachment is obsolete: true

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/bcc709f93afb
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: