browser_save_link-perwindowpb.js test might observe the wrong notifications

RESOLVED FIXED in Firefox 25

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: markh, Assigned: markh)

Tracking

Trunk
Firefox 27
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox25 fixed, firefox26 fixed, firefox27 fixed, firefox-esr24 fixed)

Details

(Whiteboard: [qa-])

Attachments

(1 attachment, 1 obsolete attachment)

Assignee

Description

6 years ago
This is a bit of an edge-case, but if some other content is loading while browser_save_link-perwindowpb.js is running, the 'http-on-examine-response' and 'http-on-modify-request' observers in that test might see that other content and will fail (eg, it will see the cookies for this other unrelated content instead of the ones the test is expecting).

It is an edge case because in the normal case, no other content will be loading.  However, in bug 896912 we are investigating issues the background thumbnailing service might cause - and if background thumbnails are active during the tests we hit this issue (the test sees the responses from the b/g thumbnails).  As it is unlikely we will *actually* enable b/g thumbnails during all tests, it's probably OK to WONTFIX this - but the change is trivial and might come back later, so I figured it was worth opening this bug anyway.

Asking Monica for review as it looks like the observer was added in bug 857427.  Please feel free to pass on to someone else if desired (Ehsan looks like another good candidate :)
Attachment #804167 - Flags: review?(mmc)
Comment on attachment 804167 [details] [diff] [review]
check the observer is actually for the page being tested

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

Thanks. I think it is a good idea to fix this ambiguity even if it hasn't triggered yet.
Attachment #804167 - Flags: review?(mmc) → review+
Assignee

Comment 2

6 years ago
Thanks!

https://hg.mozilla.org/integration/fx-team/rev/4ae90cb4b89a
Assignee: nobody → mhammond
Status: NEW → ASSIGNED
Assignee

Comment 4

6 years ago
This was broken by the moving of the tests from browser/base/test to browser/base/test/general.  This required changing the 2 lines from:

+    if (channel.URI.spec != "http://mochi.test:8888/browser/browser/base/content/test/bug792517.sjs") {

to:

+    if (channel.URI.spec != "http://mochi.test:8888/browser/browser/base/content/test/general/bug792517.sjs") {

Given this is a trivial change, carrying r+ forward.

https://hg.mozilla.org/integration/fx-team/rev/47b85389c55c
Attachment #804167 - Attachment is obsolete: true
Attachment #809549 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/47b85389c55c
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 27
Is this something that would be sensible to uplift to the release branches or just let it ride Fx27? It's test-only, so approval isn't needed.
I am not finding any oranges associated with this test, so I think we can avoid the work of uplifting.
Discussed on IRC and decided to take it after all.
Flags: needinfo?(ryanvm)
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.