1320663 - Stop using timers in PrivateBrowsingUtils.jsm

Status: RESOLVED FIXED

(Firefox :: Private Browsing, defect)

Description

[Mike de Boer [:mikedeboer]]

In the `PrivateBrowsingUtils::whenHiddenPrivateWindowReady` method, you'll see that we're using `setTimeout` to wait for a private browsing window to be ready to use.

A DXR search for this method shows that it's only used in 'docshell/test/chrome/test_private_hidden_window.html' and a search through 'https://dxr.mozilla.org/addons/' shows that no addon seems to be using the API.

Therefore it's safe to
1. add a more deterministic method to 'test_private_hidden_window.html', using the 'readystatechange' event,
2. remove this method from PrivateBrowsingUtils.jsm,
3. Confirm this is working as expected by pushing your changes to Try.

Comment 1

[ksh.shrm1]

Hi Mike,
I want to try this bug. Can you help me how to get started?

Comment 2

[Mike de Boer [:mikedeboer]]

Hi! Thanks for taking this on. The first step is to get you set up with a development environment and get you to build Firefox from source; if you've done that already, great! If not, please take a look at https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_build

If you need help getting started, ask in the #introduction channel on irc.mozilla.org (if you don't have an IRC client, there's a web-based one at https://www.irccloud.com/#!/ircs://irc.mozilla.org:6697/%23introduction or http://client01.chat.mibbit.com/?server=irc.mozilla.org/#introduction).

Once you're set up, it's time to work on the three points I mentioned in comment 0. I'm able to take care of step 3, because you need Level 1 access to be able to push to the Try server(s).
If you find yourself stuck or find a problem you can't solve, please feel free to ask me! You can do that here or on the #developers channel on IRC (my nick is 'mikedeboer' there).

Most importantly: have fun!

Comment 3

[ksh.shrm1]

I have built the source and run the tests. How to find the location of relevant code for this bug?
I used dxr but could not find the function in firefox tree.
And can you please elaborate what do you mean by a more deterministic method?
Thanks

Comment 4

[Aman Dwivedi]

Hi Mike! I looked at the issue and the code. I even tried removing the setTimeOut code and using 'onreadystatechange'. The code worked but it put the code in a blocking state (i.e. I had to click on the Run chrome test button on the browser manually to complete the test and close the window). Rather the test passed. I have another suggestion. I tried using looping method (which is a general concept of putting CPU in busy waiting state in OS). That worked well. I tested it also and it passed. I believe this approach can be implemented too(Checking for loading status using a loop and getting out of the loop once it is completely loaded). This would be a deterministic approach as the code will continue as soon as the document completes loading.

Comment 5

[Mike de Boer [:mikedeboer]]

(In reply to Aman Dwivedi from comment #4)
> Hi Mike! I looked at the issue and the code. I even tried removing the
> setTimeOut code and using 'onreadystatechange'. The code worked but it put
> the code in a blocking state (i.e. I had to click on the Run chrome test
> button on the browser manually to complete the test and close the window).

Thanks for checking! What I'd like you to try is to:

1. Remove the `whenHiddenPrivateWindowReady` method from PrivateBrowsingUtils.jsm entirely.
2. 'test_private_hidden_window.html' is the only place where `whenHiddenPrivateWindowReady` is used. Now that you removed this method, you need to check if the hidden window has loaded inside that file. To do that you need to do the following:
2.1 Check if the 'readystate' property on `hidden.document` is not already 'complete' or 'interactive'. If it already is, you can make the test continue.
2.2 Otherwise, add an 'onreadystatechange' event listener on `hidden.document` and in the handler function, check if the 'readyState' property on `hidden.document` changed to 'complete' or 'interactive'. If it did, remove the event listener from `hidden.document` and make the test continue.

> Rather the test passed. I have another suggestion. I tried using looping
> method (which is a general concept of putting CPU in busy waiting state in
> OS). That worked well. I tested it also and it passed. I believe this
> approach can be implemented too(Checking for loading status using a loop and
> getting out of the loop once it is completely loaded). This would be a
> deterministic approach as the code will continue as soon as the document
> completes loading.

It's deterministic, but if we can use event listeners when possible, we do that. I think you'll be able to make it work when you follow the steps I listed above.

Once you've attached a patch to the bug, I'll assign it to you. Feel free to request feedback or needinfo from me when you need more help.

Comment 6

[Aman Dwivedi]

OK I understood that. I removed the function. Ran the test. It is working.
How to approach for 2.1 as mentioned by you ('readystate' property on hidden.document)? I use "./mach test docshell/test/chrome/test_private_hidden_window.html" command for testing.

Comment 7

[Aman Dwivedi]

Attachment: Removed timer from PrivateBrowsingUtils.jsm and added a event listener in 'test_private_hidden_window.html'

Comment 8

[aakashpraliya13]

Attachment: fixes Bug 1320663

I think the patch submitted by Aman Dwivedi doesn't wait for hidden.document DOM to load. It seems to me that the function 'hiddenIframe' will never be called. I have made some corrections here.

Comment 9

[Mike de Boer [:mikedeboer]]

Comment on attachment 8817798 [details] [diff] [review]
fixes Bug 1320663

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

Both patches are not really what I'm looking for... There shouldn't be a need to wait for the base document' DOMContentReady event.
Instead, I'm expecting you to use only `addEventListener` to listen for events, check for 'interactive' or 'complete' ready-states - thus ignoring others. There's no need to add the event listener if the ready-state is already 'complete' or 'interactive'; in that case you can continue to create the iframe right away.
I'm also expecting nicely indented code ;-)

Comment 10

[Aman Dwivedi]

Attachment: BugFix.patch

Comment 11

[Mike de Boer [:mikedeboer]]

Comment on attachment 8819437 [details] [diff] [review]
BugFix.patch

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

The code looks good!

The only thing left to do for you here is to format it properly according to the style described here: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style

Please resubmit the patch for review after you're done that!

::: docshell/test/chrome/test_private_hidden_window.html
@@ +29,5 @@
>  // We need to wait for the hidden window to load, but can't access
>  // an event target for a regular event listener.
>  var hidden = mainWindow.Services.appShell.hiddenPrivateDOMWindow;
> +
> +function hiddenIframe(){

Can you rename this to `hiddenDocumentReady`?

Comment 12

[Aman Dwivedi]

Attachment: Bug 1320663.patch

Thanks Mike! I read the document and have tried to follow the mentioned rules of formatting and indentation. Hope this patch works. :)

Comment 13

[Mike de Boer [:mikedeboer]]

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

Comment 14

[Mike de Boer [:mikedeboer]]

Comment on attachment 8820039 [details] [diff] [review]
Bug 1320663.patch

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

That's looking very good now! You got an r+, but I can only land it for you once you uploaded a new version of your patch which addresses the comments I have below.
I also pushed your patch the the Try infrastructure, which runs all the tests to see if they all pass with your patch applied. If they do, I can land it for you and you'll have made you first contribution to Firefox!

::: docshell/test/chrome/test_private_hidden_window.html
@@ +29,5 @@
>  // We need to wait for the hidden window to load, but can't access
>  // an event target for a regular event listener.
>  var hidden = mainWindow.Services.appShell.hiddenPrivateDOMWindow;
> +
> +function hiddenDocumentReady() {

nit: on second thought, can you rename this to 'onHiddenDoumentReady', please? Sorry to make you rename it again!

@@ +44,5 @@
> +
> +if (hidden.document.readyState == "interactive" || hidden.document.readyState == "complete") {
> +  hiddenDocumentReady();
> +} else {
> +  hidden.document.addEventListener('readystatechange', function IframeListen() {

nits: please use double-quotes everywhere in the code you're adding. Please rename 'IframeListen' to just 'listener'.

@@ +49,5 @@
> +    if (hidden.document.readyState == "interactive" || hidden.document.readyState == "complete") {
> +      hidden.document.removeEventListener('readystatechange', IframeListen, false);
> +      hiddenDocumentReady();
> +    }
> +  }, false);

No need to to add `, false`, because that's the default value when you omit it.

Comment 15

[Aman Dwivedi]

Attachment: Bug 1320663.patch

Changed the things as told by you. Please check.

Comment 16

[Mike de Boer [:mikedeboer]]

Three points that will help you on your next patches too:
1. When you upload a patch to replaces an older one, you need to mark the old one as 'obsolete'. This makes sure that the bug doesn't get cluttered with a long list of patches.
2. When you want to the reviewer to take another look at your patch, it's best to re-request review to make sure you get it!
3. Make sure to add a Commit message and your User ID. Please check https://bug269442.bmoattachments.org/attachment.cgi?id=8758242 for a basic example.

Thanks!

Comment 17

[Mike de Boer [:mikedeboer]]

Seems like your test doesn't work and times out on all platforms. Did you run it yourself?

You can do that using the command './mach test docshell/test/chrome/test_private_hidden_window.html'

Comment 18

[Aman Dwivedi]

I ran the test. Seems the 'win.addEventListener' is not working. I need to click on the button "Run Test" in the opened window, then it runs the test and closes the window after that. What may be the problem? How shall I fix this?

Comment 19

[Aman Dwivedi]

I tried to find the issue by adding console.log checkpoints at various places in the code. When I ran the test, the control entered inside the else block (maybe since hidden.document.readyState was neither "interactive" nor "complete"). Then, nothing happened until I clicked on the "Run tests" button on the opened browser (i.e. the 'readystatechange' event listener didn't work at that moment). When I clicked on the button, the control again checks the first "if block" (instead of checking the if condition inside the 'readystatechange' event listener). The control then goes into the "onHiddenDocumentReady()" block and then runs as it should run. I believe the problem lies in the 'readystatechange' event listener code in the 'else block'. That part of code is not working as it should. The handler function of the 'readystatechange' is never called on "load".

Comment 20

[Aman Dwivedi]

Hi Mike! Is there anything we can do with this?

Comment 21

[Mike de Boer [:mikedeboer]]

Well, there are two things you can try:

1. Try using the 'load' event, instead of 'readystatechange', because it might work on the hidden window doument.
2. If that doesn't work, use the timer-based solution that was used in PrivateBrowsingUtils.jsm before. It'd be sad if we need to revert back to it, but at least it will only exist in testing code, not shipping code.

Comment 22

[Aman Dwivedi]

Attachment: tip.patch

I tried using "load" event, but that too didn't work. I have done it the second way and is working well. Do I need to import "resource://gre/modules/Timer.jsm" too? Because it works both ways with or without it.

Comment 23

[Mike de Boer [:mikedeboer]]

Comment on attachment 8823179 [details] [diff] [review]
tip.patch

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

Thanks Aman, for the quick turn-around!

r=me with the comments below addressed! Please upload an updated patch here and I will push it to try and after that land it on mozilla-central.

::: docshell/test/chrome/test_private_hidden_window.html
@@ +38,5 @@
> +    if (isNotLoaded()) {
> +      setTimeout(poll, 100);
> +      return;
> +    }
> +    cb(hidden);

This should be `onHiddenPrivateWindowReady()` - you don't need to pass in the hidden window as function argument, because it's declared globally.

@@ +41,5 @@
> +    }
> +    cb(hidden);
> +  }, 4);
> +} else {
> +  cb(hidden);

Same here.

@@ +44,5 @@
> +} else {
> +  cb(hidden);
> +}
> +
> +function cb(hidden) {

Please rename this method to `onHiddenPrivateWindowReady` and remove the first argument declaration.

Comment 24

[Mike de Boer [:mikedeboer]]

There's no need to import Timers.jsm, because the script is running in a window, where `setTimeout` is readily available.

Comment 25

[Aman Dwivedi]

Attachment: bugfix.patch

Hope this works! :)

Comment 26

[Mike de Boer [:mikedeboer]]

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

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c779eb0611cc
Stop using timers in PrivateBrowsingUtils.jsm, r=mikedeboer

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c779eb0611cc