Closed Bug 915961 Opened 8 years ago Closed 8 years ago

browser_save_link-perwindowpb.js test might observe the wrong notifications

Categories

(Firefox :: Private Browsing, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 27
Tracking Status
firefox25 --- fixed
firefox26 --- fixed
firefox27 --- fixed
firefox-esr24 --- fixed

People

(Reporter: markh, Assigned: markh)

Details

(Whiteboard: [qa-])

Attachments

(1 file, 1 obsolete file)

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+
Thanks!

https://hg.mozilla.org/integration/fx-team/rev/4ae90cb4b89a
Assignee: nobody → mhammond
Status: NEW → ASSIGNED
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: 8 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.