Stop using timers in PrivateBrowsingUtils.jsm

RESOLVED FIXED in Firefox 53

Status

()

defect
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: mikedeboer, Assigned: dwivedi.aman96, Mentored)

Tracking

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

50 Branch
Firefox 53
Points:
---

Firefox Tracking Flags

(firefox53 fixed)

Details

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

Attachments

(1 attachment, 6 obsolete attachments)

(Reporter)

Description

2 years 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

2 years ago
Whiteboard: [good second bug][lang=js]
(Reporter)

Updated

2 years ago
Whiteboard: [good second bug][lang=js] → [good first bug][lang=js]

Comment 1

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

Comment 2

2 years 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

2 years 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

2 years 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

2 years 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

2 years 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)

Comment 8

2 years ago
Posted patch fixes Bug 1320663 (obsolete) — Splinter Review
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

2 years 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

2 years ago
Posted patch BugFix.patch (obsolete) — Splinter Review
Attachment #8819437 - Flags: review?(mdeboer)
(Reporter)

Comment 11

2 years 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

2 years ago
Attachment #8817798 - Attachment is obsolete: true
(Reporter)

Updated

2 years ago
Attachment #8817777 - Attachment is obsolete: true
(Assignee)

Comment 12

2 years ago
Posted patch Bug 1320663.patch (obsolete) — Splinter Review
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

2 years ago
Assignee: nobody → dwivedi.aman96
Status: NEW → ASSIGNED
(Reporter)

Comment 14

2 years 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

2 years ago
Posted patch Bug 1320663.patch (obsolete) — Splinter Review
Changed the things as told by you. Please check.
(Reporter)

Updated

2 years ago
Attachment #8820039 - Attachment is obsolete: true
(Reporter)

Updated

2 years ago
Attachment #8819437 - Attachment is obsolete: true
(Reporter)

Updated

2 years ago
Attachment #8820334 - Flags: review+
(Reporter)

Comment 16

2 years 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

2 years 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

2 years 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

2 years 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

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

Comment 21

2 years 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

2 years ago
Posted patch tip.patch (obsolete) — Splinter Review
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

2 years 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

2 years 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

2 years ago
Posted patch bugfix.patchSplinter Review
Hope this works! :)
Attachment #8823179 - Attachment is obsolete: true
Attachment #8823213 - Flags: review?(mdeboer)
(Reporter)

Updated

2 years ago
Attachment #8823213 - Flags: review?(mdeboer) → review+
(Reporter)

Updated

2 years ago
Keywords: checkin-needed

Comment 27

2 years 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

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