The test browser_bug666317.js fails when e10s is enabled, reporting: 0:06.04 TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/image/test/browser/browser_bug666317.js | Image should be discarded. I did a little debugging here. The memory-pressure notification makes its way down to the child process, and the 2 observers in imgLoader.cpp are called in both the parent and child processes, but still the test reports failure. I instrumented the test to use 1 second setTimeout() after sending the notification under the assumption that the cross-process forwarding of that notification introduced latency the test wasn't expecting, but the problem remains.

Comment on attachment 8469690 [details] [diff] [review] Bug 948194: Re-write the test so that checking if the image is decoded takes place in the child allowing for the `memory-pressure` notification to be processed. Review of attachment 8469690 [details] [diff] [review]: ----------------------------------------------------------------- Tell me if you disagree with my comment... Also, don't forget to change image/test/browser/browser.ini to not skip this test if we're e10s! I'll check in the next patch I see here. ::: image/test/browser/browser_bug666317.js @@ +17,5 @@ > + let request = img.getRequest(Components.interfaces.nsIImageLoadingContent.CURRENT_REQUEST); > + let result = request.imageStatus & Components.interfaces.imgIRequest.STATUS_FRAME_COMPLETE ? true : false; > + > + let msg_name = "test666317:isImgDecoded:Answer_Type_" + message.data.test; > + message.target.sendAsyncMessage(msg_name, { result: result }); It'd probably be a little easier to have a single message and pass which test we're responding to in the "data" object.

Comment on attachment 8470336 [details] [diff] [review] Bug 948194: Re-write the test so that checking if the image is decoded takes place in the child allowing for the `memory-pressure` notification to be processed. Review of attachment 8470336 [details] [diff] [review]: ----------------------------------------------------------------- ::: image/test/browser/browser.ini @@ -7,5 @@ > image.html > imageX2.html > - > -[browser_bug666317.js] > -skip-if = e10s # Bug 948194 - Decoded Images seem to not be discarded on memory-pressure notification with e10s enabled Oops, you only want to remove the skip-if line. Otherwise, we won't run these tests at all.

Sorry about that. I didn't even notice those lines.

Moving old M2 P2 bugs to M4.

Move old M2's low-priority bugs to M6 milestone.

oops: old M2 P2 bugs should have been moved to new M4 milestone, not M6.

Blake, looks like this just needs to land? Tried ni'ing atifrea@mozilla.com but that account is dead.

Yes, my old account is dead. My current one is alex.tifrea93@gmail.com

I went to check this patch in to find that it had bitrotted badly. In un-bitrotting it, I ran into two bugs that were worth talking about. The first is that on e10s only, tab switching no longer sends all of its notifications and events synchronously. This meant that the pattern |gBrowser.selectedTab = oldTab; /* expect newTab to be inactive */| was no longer correct in e10s. The second is that now that we run the event loop (and send a low-memory notification) the chances of collecting the scripted image observer were much higher. I fixed this by holding a strong reference to it in the JS, but I'm a little worried that this will be a footgun in the future. I guess the two options are to hold a strong reference to it from imagelib and depend on JS calling cancelAndForgetObserver (or forgetting and leaking) or the opposite (and crashing if JS forgets). Seth, your comments are welcome :) I'll attach a diff -w as well, since that's much easier to read.

Comment on attachment 8528542 [details] [diff] [review] patch -w Review of attachment 8528542 [details] [diff] [review]: ----------------------------------------------------------------- Looks good! As I mentioned in person, it might be wise to push a try job rebased on top of the current mozilla-central, since some notification-related changes just landed. ::: image/test/browser/browser_bug666317.js @@ +37,4 @@ > > // Clone the current imgIRequest with our new observer. > let request = currentRequest(); > + return [ request.clone(scriptedObserver), scriptedObserver ]; I'd suggest adding a comment here so readers know that we're doing this to make sure we have a live reference to the scriptedObserver. (I'm sure in a year I'll have forgotten what's going on here.)

Comment on attachment 8528541 [details] [diff] [review] Rebased patch Review of attachment 8528541 [details] [diff] [review]: ----------------------------------------------------------------- (r+'ing based on the other version of the patch.)

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/df5c7392ecbf, apparently it's still a bit racy. https://treeherder.mozilla.org/logviewer.html#?job_id=4246502&repo=mozilla-inbound https://treeherder.mozilla.org/logviewer.html#?job_id=4246633&repo=mozilla-inbound https://treeherder.mozilla.org/logviewer.html#?job_id=4243982&repo=mozilla-inbound https://treeherder.mozilla.org/logviewer.html#?job_id=4245287&repo=mozilla-inbound https://treeherder.mozilla.org/logviewer.html#?job_id=4246996&repo=mozilla-inbound https://treeherder.mozilla.org/logviewer.html#?job_id=4242575&repo=mozilla-inbound https://treeherder.mozilla.org/logviewer.html#?job_id=4245378&repo=mozilla-inbound

This test is now disabled entirely for all platforms. I'm not going to work on it until we figure out how to make it work reliably.

Moving to p3 because no activity for at least 1 year(s). See https://github.com/mozilla/bug-handling/blob/master/policy/triage-bugzilla.md#how-do-you-triage for more information