969779 - [b2g emulator][mochitest] Should use |SpecialPowers.pushPrefEnv()| in image/test/mochitest/test_bug399925.html

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

image/test/mochitest/test_bug399925.html is failed on b2g emulator test because content process is not allowed to set preference directly via nsIPrefService. We should use SpecialPowers.pushPrefEnv() instead.

Comment 1

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

log from try server:

>05:17:31     INFO -  394 INFO TEST-START | /tests/image/test/mochitest/test_bug399925.html
>05:17:31     INFO -  [Child 750] WARNING: We don't support transparent content with displayports, force it to be opqaue: file ../../../gecko/layout/base/nsDisplayList.cpp, line 1220
>05:17:31     INFO -  [Child 750] WARNING: We don't support transparent content with displayports, force it to be opqaue: file ../../../gecko/layout/base/nsDisplayList.cpp, line 1220
>05:17:31     INFO -  [Parent 692] WARNING: PRFileDescAutoLock cannot get fd!: 'mFd', file ../../../../gecko/netwerk/base/src/nsSocketTransport2.h, line 195
>05:17:31     INFO -  [Child 750] WARNING: We don't support transparent content with displayports, force it to be opqaue: file ../../../gecko/layout/base/nsDisplayList.cpp, line 1220
>05:17:31     INFO -  [Child 750] WARNING: We don't support transparent content with displayports, force it to be opqaue: file ../../../gecko/layout/base/nsDisplayList.cpp, line 1220
>05:17:31     INFO -  [Child 750] ###!!! ASSERTION: ENSURE_MAIN_PROCESS failed. Cannot SetBoolPref from content process: discardable: 'Error', file ../../../../gecko/modules/libpref/src/nsPrefBranch.cpp, line 153
>05:17:50     INFO -  395 ERROR TEST-UNEXPECTED-FAIL | /tests/image/test/mochitest/test_bug399925.html | uncaught exception - Error: Permission denied to access property 'toString' at :0
>05:17:50     INFO -  JavaScript error: , line 0: Permission denied to access property 'toString'
>05:17:50     INFO -  396 ERROR TEST-UNEXPECTED-FAIL | /tests/image/test/mochitest/test_bug399925.html | uncaught exception - Error: Permission denied to access property 'message' at :0
>05:17:50     INFO -  JavaScript error: , line 0: Permission denied to access property 'message'
>05:17:50     INFO -  397 ERROR TEST-UNEXPECTED-FAIL | /tests/image/test/mochitest/test_bug399925.html | uncaught exception - uncaught exception: unknown (can't convert to string) at :0
>05:17:50     INFO -  JavaScript error: , line 0: uncaught exception: unknown (can't convert to string)
>05:17:50     INFO -  [Child 750] WARNING: We don't support transparent content with displayports, force it to be opqaue: file ../../../gecko/layout/base/nsDisplayList.cpp, line 1220
>05:17:50     INFO -  398 INFO TEST-INFO | MEMORY STAT vsize after test: 114855936
>05:17:50     INFO -  399 INFO TEST-INFO | MEMORY STAT residentFast after test: 61104128
>05:17:50     INFO -  400 INFO TEST-INFO | MEMORY STAT heapAllocated after test: 17761720
>05:17:50     INFO -  401 ERROR TEST-UNEXPECTED-FAIL | /tests/image/test/mochitest/test_bug399925.html | [SimpleTest.finish()] this test already called finish!
>05:17:50     INFO -  402 ERROR TEST-UNEXPECTED-FAIL | /tests/image/test/mochitest/test_bug399925.html | called finish() multiple times
>05:17:50     INFO -  403 ERROR TEST-UNEXPECTED-FAIL | /tests/image/test/mochitest/test_bug399925.html | [SimpleTest.finish()] this test already called finish!
>05:17:50     INFO -  404 ERROR TEST-UNEXPECTED-FAIL | /tests/image/test/mochitest/test_bug399925.html | called finish() multiple times
>05:17:50     INFO -  [Child 750] WARNING: We don't support transparent content with displayports, force it to be opqaue: file ../../../gecko/layout/base/nsDisplayList.cpp, line 1220
>05:17:50     INFO -  405 INFO TEST-END | /tests/image/test/mochitest/test_bug399925.html | finished in 7888ms
>05:17:50     INFO -  [Child 750] WARNING: We don't support transparent content with displayports, force it to be opqaue: file ../../../gecko/layout/base/nsDisplayList.cpp, line 1220
>05:17:50     INFO -  406 ERROR TEST-UNEXPECTED-FAIL | /tests/image/test/mochitest/test_bug399925.html | Assertion count 1 is greater than expected range 0-0 assertions.

Comment 2

[Junior [inactive]]

Attachment: apply-special-powers-for-oop.patch

1. Use |SpecialPowers.pushPrefEnv| to enable preference setting in oop
2. Let |setImagePref| in imgutils.js be able to used in oop
3. Modify the assertion to make the comparison non-trivial

try result: https://tbpl.mozilla.org/?tree=Try&rev=58d4b40e90c4

Comment 3

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8414245 [details] [diff] [review]
apply-special-powers-for-oop.patch

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

::: image/test/mochitest/imgutils.js
@@ +93,1 @@
>  }

Are these changes still necessary after addressing my comment in test_bug399925?

::: image/test/mochitest/test_bug399925.html
@@ +33,5 @@
> +    'set':[['image.mem.discardable',true],
> +           // Sets the discard timer to 500ms so we don't have to wait so long. The
> +           // actual time is nondeterministic, but is bounded by 2 * timer = 1 second.
> +           ['image.mem.in_discard_timeout_ms',1000]]
> +	}, function(){

|pushPrefEnv| is the precondition of this test case. I'll suggest to move this part of code out of |runTest|. You may refer to http://dxr.mozilla.org/mozilla-central/source/dom/indexedDB/ipc/test_ipc.html#168.

@@ +46,3 @@
>  
> +    // Set the timeout to draw it after discard
> +    setTimeout('drawCanvas(); allDone();', 3000);

Is there any chance we can replace this |setTimeout| with some code more deterministic?

@@ +63,5 @@
>  function allDone()
>  {
>      is(pngResults[0], pngResults[1], "got different rendered results");
>      setImagePref(DISCARD_TIMEOUT_PREF, oldTimeoutPref);
>      setImagePref(DISCARD_ENABLED_PREF, oldDiscardPref);

Why we still need these two |setImagePref| after using |pushPrefEnv|?

Comment 4

[Junior [inactive]]

Attachment: Apply specialpowers for pref setting in oop

Update according to feedback comments.

try result: https://tbpl.mozilla.org/?tree=Try&rev=52979a818921

Comment 5

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8414978 [details] [diff] [review]
Apply specialpowers for pref setting in oop

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

Good job on finding out a deterministic way for testing image discarding. Here are some more comments.

::: image/test/mochitest/test_bug399925.html
@@ +31,5 @@
> +
> +  SpecialPowers.pushPrefEnv({
> +    'set':[['image.mem.discardable',true],
> +           ['image.mem.min_discard_timeout_ms',1000]]
> +	}, runTest);

nit: replace tab with space.

@@ +37,5 @@
>  
>  function runTest()
>  {
> +  //Redraw and compare the results if after discard
> +  var gif = document.getElementById('gif');

This |gif| element is created in line #49, so this variable should be undefined. Shouldn't these codes be placed after line #52, after the image element is appended to document?

@@ +89,2 @@
>  {
> +  is(pngResults[0], pngResults[1], "got different rendered results");

You can put this line of code in line #44 because it is only being used one time.

Comment 6

[Junior [inactive]]

Attachment: apply-special-powers-for-oop.patch

Update according to feedback comments.

Here's something need to be clairfied.

@@ +37,5 @@
>  
>  function runTest()
>  {
> +  //Redraw and compare the results if after discard
> +  var gif = document.getElementById('gif');

Actually, there's a HTML element also with id 'gif', thus causing ambiguity.
Therefore, I rename the ids in this patch.

The element |jsCreateImg| should be the target element to see if it is discardable. The behaviour is different from the original test. I have modified it in this patch and I am wondering if my understanding is right or not.

Moreover, I found that:
1. Both |jsCreateImg| and |htmlImg| are discardable and affected by the preferences.
2. A weird result: If I remove the |htmlImg| (i.e. remove line #16), the discardable callback will not be invoked.

try result:  https://tbpl.mozilla.org/?tree=Try&rev=90eae040a4ca

Comment 7

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8415188 [details] [diff] [review]
apply-special-powers-for-oop.patch

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

Please see my comments in below:

::: image/test/mochitest/test_bug399925.html
@@ +12,5 @@
>  <body>
>  <a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=399925">Mozilla Bug 399925</a>
>  <p id="display"></p>
>  <div id="content" style="display: none">
> +<img src="bug399925.gif" id="htmlImg" />

This <img> isn't used anywhere in the entire test case. This <img> is introduced in bug 399925 and the JS-created <img> is introduced in bug 478398. Can you find out if this <img> is still necessary for this test case? Please add a comment for it if there is a reason.

@@ +24,4 @@
>  SimpleTest.waitForExplicitFinish();
> +
> +window.onload = function()
> +{

nit: no need new line for |{|

@@ +37,3 @@
>  
>  function runTest()
>  {

nit: no need new line for |{|

@@ +56,3 @@
>  
> +function addDiscardCallback(anImage, callback)
> +{

nit: no need new line for |{|

@@ +74,4 @@
>  }
>  
>  function drawCanvas()
>  {

nit: no need new line for |{|

Comment 8

[Junior [inactive]]

Attachment: apply-special-powers-for-oop.patch

1. remove a new line for |{| following by a function
2. remove <img> and roll back the id renaming
3. move the first draw canvas to the loadComplete callback

About the change of #2 & #3:
As mentioned, if I remove the |htmlImg| (i.e. remove line #16), the discardable callback will not be invoked.
The reason is that the image |bug399925.gif| is not loaded.
Therefore, |drawCanvas| will append an empty image. That's why I made change of #3.

Moreover, since everything is after |window.onload|, all the images including |bug399925.gif| have finished loading if there is a |<img src="bug399925.gif" />|.

try result: https://tbpl.mozilla.org/?tree=Try&rev=7dc23395e221

Comment 9

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8417182 [details] [diff] [review]
apply-special-powers-for-oop.patch

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

Well done! lgtm.

Comment 10

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

Comment on attachment 8417182 [details] [diff] [review]
apply-special-powers-for-oop.patch

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

Nicely done!

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/0497635fa226

For future patches, please use your full name in the hg commit information. Thanks :)

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0497635fa226

Comment 13

[Junior [inactive]]

Thanks for your notice.