Stop using timers in PrivateBrowsingUtils.jsm

RESOLVED FIXED in Firefox 53

Status

()

Firefox
Private Browsing
RESOLVED FIXED
6 months ago
5 months ago

People

(Reporter: mikedeboer, Assigned: Aman Dwivedi, Mentored)

Tracking

(Depends on: 1 bug, {good-first-bug})

50 Branch
Firefox 53
good-first-bug
Points:
---

Firefox Tracking Flags

(firefox53 fixed)

Details

(Whiteboard: [good first bug][lang=js])

Attachments

(1 attachment, 6 obsolete attachments)

(Reporter)

Description

6 months ago
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.
(Reporter)

Updated

6 months ago
Whiteboard: [good second bug][lang=js]
(Reporter)

Updated

6 months ago
Whiteboard: [good second bug][lang=js] → [good first bug][lang=js]

Comment 1

6 months ago
Hi Mike,
I want to try this bug. Can you help me how to get started?
Flags: needinfo?(mdeboer)
(Reporter)

Comment 2

6 months ago
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!
Flags: needinfo?(mdeboer) → needinfo?(ksh.shrm1)

Comment 3

6 months ago
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
Flags: needinfo?(ksh.shrm1) → needinfo?(mdeboer)
(Assignee)

Comment 4

6 months ago
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.
(Reporter)

Comment 5

6 months ago
(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.
Flags: needinfo?(mdeboer)
(Assignee)

Comment 6

6 months ago
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.
Flags: needinfo?(mdeboer)
(Assignee)

Comment 7

6 months ago
Created attachment 8817777 [details] [diff] [review]
Removed timer from PrivateBrowsingUtils.jsm and added a event listener in 'test_private_hidden_window.html'
Flags: needinfo?(mdeboer)

Comment 8

6 months ago
Created attachment 8817798 [details] [diff] [review]
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.
Attachment #8817798 - Flags: review?(mdeboer)
(Reporter)

Comment 9

5 months ago
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 ;-)
Attachment #8817798 - Flags: review?(mdeboer)
(Assignee)

Comment 10

5 months ago
Created attachment 8819437 [details] [diff] [review]
BugFix.patch
Attachment #8819437 - Flags: review?(mdeboer)
(Reporter)

Comment 11

5 months ago
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`?
Attachment #8819437 - Flags: review?(mdeboer) → review-
(Reporter)

Updated

5 months ago
Attachment #8817798 - Attachment is obsolete: true
(Reporter)

Updated

5 months ago
Attachment #8817777 - Attachment is obsolete: true
(Assignee)

Comment 12

5 months ago
Created attachment 8820039 [details] [diff] [review]
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. :)
Attachment #8820039 - Flags: review?(mdeboer)
(Reporter)

Updated

5 months ago
Assignee: nobody → dwivedi.aman96
Status: NEW → ASSIGNED
(Reporter)

Comment 13

5 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0e2b9b6e2305
(Reporter)

Comment 14

5 months ago
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.
Attachment #8820039 - Flags: review?(mdeboer) → review+
(Assignee)

Comment 15

5 months ago
Created attachment 8820334 [details] [diff] [review]
Bug 1320663.patch

Changed the things as told by you. Please check.
(Reporter)

Updated

5 months ago
Attachment #8820039 - Attachment is obsolete: true
(Reporter)

Updated

5 months ago
Attachment #8819437 - Attachment is obsolete: true
(Reporter)

Updated

5 months ago
Attachment #8820334 - Flags: review+
(Reporter)

Comment 16

5 months ago
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!
(Reporter)

Comment 17

5 months ago
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'
Flags: needinfo?(dwivedi.aman96)
(Assignee)

Comment 18

5 months ago
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?
Flags: needinfo?(dwivedi.aman96)
(Assignee)

Comment 19

5 months ago
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".
(Assignee)

Comment 20

5 months ago
Hi Mike! Is there anything we can do with this?
Flags: needinfo?(mdeboer)
(Reporter)

Comment 21

5 months ago
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.
Flags: needinfo?(mdeboer)
(Assignee)

Comment 22

5 months ago
Created attachment 8823179 [details] [diff] [review]
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.
Attachment #8820334 - Attachment is obsolete: true
Flags: needinfo?(mdeboer)
Attachment #8823179 - Flags: review?(mdeboer)
(Reporter)

Comment 23

5 months ago
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.
Attachment #8823179 - Flags: review?(mdeboer) → review+
(Reporter)

Comment 24

5 months ago
There's no need to import Timers.jsm, because the script is running in a window, where `setTimeout` is readily available.
Flags: needinfo?(mdeboer)
(Assignee)

Comment 25

5 months ago
Created attachment 8823213 [details] [diff] [review]
bugfix.patch

Hope this works! :)
Attachment #8823179 - Attachment is obsolete: true
Attachment #8823213 - Flags: review?(mdeboer)
(Reporter)

Comment 26

5 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=13db61bebe90
(Reporter)

Updated

5 months ago
Attachment #8823213 - Flags: review?(mdeboer) → review+
(Reporter)

Updated

5 months ago
Keywords: checkin-needed

Comment 27

5 months ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c779eb0611cc
Stop using timers in PrivateBrowsingUtils.jsm, r=mikedeboer
Keywords: checkin-needed

Comment 28

5 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c779eb0611cc
Status: ASSIGNED → RESOLVED
Last Resolved: 5 months ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
You need to log in before you can comment on or make changes to this bug.