Intermittent browser_thumbnails_update.js | got notification of item being created. - Got 2, expected 1 | still only 1 notification of item being created. - Got 2, expected 1

RESOLVED FIXED in Firefox 25

Status

()

Firefox
Tabbed Browser
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: ttaubert, Assigned: markh)

Tracking

({intermittent-failure})

Trunk
Firefox 26
x86
Linux
intermittent-failure
Points:
---

Firefox Tracking Flags

(firefox24 unaffected, firefox25 fixed, firefox26 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
https://tbpl.mozilla.org/php/getParsedLog.php?id=25805782&tree=Fx-Team
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/components/thumbnails/test/browser_thumbnails_update.js | got notification of item being created. - Got 2, expected 1
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/components/thumbnails/test/browser_thumbnails_update.js | still only 1 notification of item being created. - Got 2, expected 1
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
(Assignee)

Comment 6

5 years ago
Created attachment 795801 [details] [diff] [review]
0001-Bug-898743-don-t-thumbnail-non-http-channels-with-a-.patch

I think the problem here is that our data: URL is being captured by the f/g service.  We've fixed that for tests that use the .sjs responses by setting the cache-control header.

This patch checks if a non-http channel still has HTML, and if so, queries for a http-equiv cache-control tag, and if no-store is in that, skips the capture.  This test also adds that tag.
Assignee: nobody → mhammond
Status: NEW → ASSIGNED
Attachment #795801 - Flags: review?(adw)
(Assignee)

Comment 7

5 years ago
Comment on attachment 795801 [details] [diff] [review]
0001-Bug-898743-don-t-thumbnail-non-http-channels-with-a-.patch

adw pointed out on IRC that maybe we should just use the .sjs file in that test too, and avoid adding the extra checks to browser-thumbnail, which is hard to argue with.  I'll wait and see how he goes with bug 901294 as a fix may also come out of that, otherwise I'll do a new patch over the next few days.
Attachment #795801 - Attachment is obsolete: true
Attachment #795801 - Flags: review?(adw)
(Assignee)

Comment 8

5 years ago
Created attachment 796990 [details] [diff] [review]
0001-Bug-898743-ensure-Cache-Control-no-store-for-all-thu.patch

As suggested, just use the .sjs file instead of a data: URL for the test that goes orange.
Attachment #796990 - Flags: review?(adw)
Comment on attachment 796990 [details] [diff] [review]
0001-Bug-898743-ensure-Cache-Control-no-store-for-all-thu.patch

Review of attachment 796990 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/thumbnails/test/thumbnails_update.sjs
@@ +24,5 @@
>    aResponse.setHeader("Cache-Control", "no-store");
>  
> +  // for the simple test - just return some content.
> +  if (aRequest.queryString == "simple") {
> +    aResponse.write("<html><body bgcolor=ff0000></body></html>");

The !simple case returns red and green pages, so please return a white page here (e.g., by not specifying a color at all) so that we don't confuse ourselves when we're debugging problems.  Not to mention, if there's some bug in a test that's expecting a !simple page but it ends up getting the simple page -- such a test should fail cleanly.

simpleCaptureTest says "Create a tab with a red background," but it doesn't actually matter what the background is.
Attachment #796990 - Flags: review?(adw) → review+
(Assignee)

Comment 10

5 years ago
Thanks!  Pushed with comments addressed.

https://hg.mozilla.org/integration/fx-team/rev/ed58ee0f5829
https://hg.mozilla.org/mozilla-central/rev/ed58ee0f5829
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
Is this something we should consider backporting to Aurora? Looks like it would need some adapting, though.
status-firefox24: --- → unaffected
status-firefox25: --- → affected
status-firefox26: --- → fixed
Flags: needinfo?(mhammond)
(Assignee)

Comment 13

5 years ago
Created attachment 797584 [details] [diff] [review]
Aurora version of the same patch

This is a test only version of the patch for Aurora.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 870100
User impact if declined: Oraange-factor
Testing completed (on m-c, etc.): Landed on m-c
Risk to taking this patch (and alternatives if risky): Low risk, test-only patch.
String or IDL/UUID changes made by this patch: None
Attachment #797584 - Flags: review+
Attachment #797584 - Flags: approval-mozilla-aurora?
Flags: needinfo?(mhammond)
Comment on attachment 797584 [details] [diff] [review]
Aurora version of the same patch

Test-only patches don't need approval. I'll push this soon, thanks!
Attachment #797584 - Flags: approval-mozilla-aurora?
You need to log in before you can comment on or make changes to this bug.