Closed Bug 862570 Opened 7 years ago Closed 7 years ago

Add a preference for an observer listening for "test.osfile.web-workers-shutdown".

Categories

(Toolkit :: OS.File, defect)

x86
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla23

People

(Reporter: yzen, Assigned: yzen)

References

Details

Attachments

(1 file, 5 obsolete files)

Instead of unconditionally adding a listener for "test.osfile.web-workers-shutdown" the observer should be added based on whether a preference is set or not.
Depends on: 845190
Attached patch Patch for 862570 with tests. (obsolete) — Splinter Review
Pushed to try:
https://tbpl.mozilla.org/?tree=Try&rev=2d40c3c6f5b3
Attachment #738318 - Flags: review?(dteller)
Same test, but also applying the ICU patches that reveal bug 845190
Try: https://tbpl.mozilla.org/?tree=Try&rev=1c3390debd27
Attached patch Patch v2 with tests. (obsolete) — Splinter Review
Updated based no v2 patch for Bug 845190
Attachment #738318 - Attachment is obsolete: true
Attachment #738318 - Flags: review?(dteller)
Attachment #738714 - Flags: review?(dteller)
Pushed to try with all 3 patches (both OS.File ones are v2):
https://tbpl.mozilla.org/?tree=Try&rev=c17e06ba1217
Comment on attachment 738714 [details] [diff] [review]
Patch v2 with tests.

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

Looks good.
r=me with the following changes. Also, could you fold my patch into yours? Stand-alone, my patch is useless.

::: toolkit/components/osfile/osfile_async_front.jsm
@@ +191,5 @@
>   */
>  let webWorkersShutdownObserver = function webWorkersShutdownObserver(aSubject, aTopic) {
>    if (aTopic == WEB_WORKERS_SHUTDOWN_TOPIC) {
>      Services.obs.removeObserver(webWorkersShutdownObserver, WEB_WORKERS_SHUTDOWN_TOPIC);
> +    Services.prefs.clearUserPref(PREF_OSFILE_TEST_SHUTDOWN_OBSERVER);

That's a somewhat roundabout way of doing things. I would prefer if we removed TEST_WEB_WORKERS_SHUTDOWN_TOPIC both here and when the preference is cleared (with the appropriate try/catch, of course) and did not change the preference automatically.

@@ +240,5 @@
> +      } catch (ex) {
> +        // There was no observer to remove.
> +      }
> +    }
> +  }, false);

This assumes that we are starting with PREF_OSFILE_TEST_SHUTDOWN_OBSERVER set to false. Which is probably correct, but requires some documentation.
Attachment #738714 - Flags: review?(dteller) → review+
Blocks: 845190
No longer depends on: 845190
(In reply to Yura Zenevich [:yzen] from comment #4)
> Pushed to try with all 3 patches (both OS.File ones are v2):
> https://tbpl.mozilla.org/?tree=Try&rev=c17e06ba1217

Please include the following patches in try runs:
https://hg.mozilla.org/try/rev/de1e5df16a02
https://hg.mozilla.org/try/rev/a4959fa31a7a

With these patches, the failures in test_logging.js on Windows become fully reproducible; without them, they seem fairly rare. And if you can provide any hints as to how my patches relate to the failures, that would be much appreciated!
I noticed bug 858723 is already in test suit and it affects the test case I am updating to test the preference. Should I wait until it pushed?
Flags: needinfo?(dteller)
Sounds like a good idea.
Flags: needinfo?(dteller)
Depends on: 858723
Attached patch Patch with tests v3 (obsolete) — Splinter Review
Note that I am adding a timeout of 100MS and resolve the promise anyways, since when there is no observer the listener should never get the file descriptors leaks message.
Attachment #738714 - Attachment is obsolete: true
Attachment #740647 - Flags: review?(dteller)
This patch is incremental to your patch for bug 858723.
Also, as your requested, it folds your patch for bug 845190.
Comment on attachment 740647 [details] [diff] [review]
Patch with tests v3

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

Nice patch getting nicer.

::: toolkit/components/osfile/tests/mochi/main_test_osfile_async.js
@@ +856,5 @@
>          Services.console.registerListener(listener);
>          f();
> +        // If listener does not resolve webObservation in timely manner (100MS),
> +        // resolve it anyways.
> +        timeout = setTimeout(function() { waitObservation.resolve(); }, 100);

You might wish to reject in case of timeout.
(if the promise has already been resolved, the rejection will be ignored).

In this case, surround your |yield inDebugTest(...)| with a try/catch to fail the test if the promise is rejected.

@@ +885,5 @@
> +          Services.obs.notifyObservers(null, "test.osfile.web-workers-shutdown",
> +            null);
> +        });
> +        yield openedFile.close();
> +      });

And here, pass a boolean to |testFileDescriptorLeaks| to determine whether you expect |yield inDebugTest(...)| to return normally or to throw.
Attachment #740647 - Flags: review?(dteller) → feedback+
Attached patch Patch with tests v4 (obsolete) — Splinter Review
Attachment #740647 - Attachment is obsolete: true
Attachment #740863 - Flags: review?(dteller)
Comment on attachment 740863 [details] [diff] [review]
Patch with tests v4

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

::: toolkit/components/osfile/tests/mochi/main_test_osfile_async.js
@@ +870,1 @@
>          yield waitObservation.promise;

Given that waitObservation.promise can be rejected, you should wrap it in a try/catch - and fail the test in case you end up in the |catch| block.
Attachment #740863 - Flags: review?(dteller) → feedback+
Comment on attachment 740863 [details] [diff] [review]
Patch with tests v4

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

::: toolkit/components/osfile/tests/mochi/main_test_osfile_async.js
@@ +882,5 @@
>          null);
>      });
>      yield iterator.close();
>  
> +    let testFileDescriptorsLeaks = function testFileDescriptorsLeaks(resolve) {

Btw, |resolve| is a little error-inducing here. Perhaps |shouldResolve|.
Attached patch Patch with tests v5 (obsolete) — Splinter Review
No more an incremental to bug 858723 (since it's already in).
Attachment #740863 - Attachment is obsolete: true
Attachment #741489 - Flags: review?(dteller)
Comment on attachment 741489 [details] [diff] [review]
Patch with tests v5

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

Good for me.
Could you apply the suggestion below + post to TryServer (mochitest-o)? No need to ask for another review.

::: toolkit/components/osfile/tests/mochi/main_test_osfile_async.js
@@ +865,5 @@
>          Services.console.registerListener(listener);
>          f();
> +        // If listener does not resolve webObservation in timely manner (100MS),
> +        // reject it.
> +        setTimeout(function() { waitObservation.reject(); }, 100);

Nit: Could you add a message here? Something along the lines of
  test.info("waitObservation timeout exceeded");
Attachment #741489 - Flags: review?(dteller) → review+
Carrying over r+ from previous patch revision.
Attachment #741489 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/0b68d19943c1
Assignee: nobody → yura.zenevich
Flags: in-testsuite+
Keywords: checkin-needed
Depends on: 866293
https://hg.mozilla.org/mozilla-central/rev/0b68d19943c1
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
You need to log in before you can comment on or make changes to this bug.