Test failures when reloading test_bug669671.html

RESOLVED FIXED in mozilla25

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

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

Tracking

Trunk
mozilla25
x86
macOS
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

Posted 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!
Posted 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+
Posted 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: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
You need to log in before you can comment on or make changes to this bug.