948194 - [e10s] Decoded Images seem to not be discarded on memory-pressure notification with e10s enabled

Status: NEW

(Core :: Graphics: ImageLib, defect, P3)

Description

[Mark Hammond [:markh] [:mhammond]]

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 1

[atifrea]

Attachment: 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.

Comment 2

[Blake Kaplan (:mrbkap) (inactive)]

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 3

[atifrea]

Attachment: 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.

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

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.

Comment 5

[atifrea]

Attachment: 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.

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

Comment 6

[Chris Peterson [:cpeterson]]

Moving old M2 P2 bugs to M4.

Comment 7

[Chris Peterson [:cpeterson]]

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

Comment 8

[Chris Peterson [:cpeterson]]

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

Comment 9

[Jim Mathies [:jimm]]

Blake, looks like this just needs to land?

Tried ni'ing [e-mail] but that account is dead.

Comment 10

[Alexandru Tifrea]

Yes, my old account is dead. My current one is [e-mail]

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Rebased patch

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 12

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: patch -w

Comment 13

[Seth Fowler [:seth] [:s2h]]

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 14

[Seth Fowler [:seth] [:s2h]]

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

Comment 15

[Blake Kaplan (:mrbkap) (inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=feacc6bbdf74

Comment 16

[Blake Kaplan (:mrbkap) (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/01093b5a774b

Comment 17

[Phil Ringnalda (:philor)]

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

Comment 18

[Blake Kaplan (:mrbkap) (inactive)]

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.

Comment 19

[Sylvestre Ledru [:Sylvestre]]

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