915961 - browser_save_link-perwindowpb.js test might observe the wrong notifications

Status: RESOLVED FIXED

(Firefox :: Private Browsing, defect)

Description

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

Attachment: check the observer is actually for the page being tested

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 :)

Comment 1

[[:mmc] Monica Chew (no longer reading bugmail)]

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.

Comment 2

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

Thanks!

https://hg.mozilla.org/integration/fx-team/rev/4ae90cb4b89a

Comment 3

[Ryan VanderMeulen [:RyanVM]]

Backed out for mochitest-bc failures.
https://hg.mozilla.org/integration/fx-team/rev/f99badef4ed5

https://tbpl.mozilla.org/php/getParsedLog.php?id=28260406&tree=Fx-Team

Comment 4

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

Attachment: 0001-Bug-915961-browser_save_link-perwindowpb.js-now-chec.patch

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

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/47b85389c55c

Comment 6

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 7

[[:mmc] Monica Chew (no longer reading bugmail)]

I am not finding any oranges associated with this test, so I think we can avoid the work of uplifting.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Discussed on IRC and decided to take it after all.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/cc7e304e596d
https://hg.mozilla.org/releases/mozilla-beta/rev/cfad534080e6
https://hg.mozilla.org/releases/mozilla-esr24/rev/7a5b6c45e112

Comment 10

[Ryan VanderMeulen [:RyanVM]]

*sigh* And then I went and pushed the version that had the paths *with* /general in them. Fail.
https://hg.mozilla.org/releases/mozilla-aurora/rev/cd4ec57115ab
https://hg.mozilla.org/releases/mozilla-beta/rev/d8d849e0b46f
https://hg.mozilla.org/releases/mozilla-esr24/rev/4f2424078c27