Show notifications on startup

Show notifications on startup

[Felix Dahlke, 2013-07-19 13:53:52]

Uploaded a review with the getLocalizedTexts tests:

http://codereview.adblockplus.org/11174072/

http://codereview.adblockplus.org/11165026/diff/1/lib/ui.js
File lib/ui.js (right):

http://codereview.adblockplus.org/11165026/diff/1/lib/ui.js#newcode425
lib/ui.js:425: let timer =
Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);
I wasn't sure if we have to cancel this timer under certain circumstances, do
we?

[Wladimir Palant, 2013-07-19 15:17:52]

http://codereview.adblockplus.org/11165026/diff/1/lib/notification.js
File lib/notification.js (right):

http://codereview.adblockplus.org/11165026/diff/1/lib/notification.js#newcode54
lib/notification.js:54: let defaultLocale = "en";
Actually, our default locale is always en-US - and that's the only reason why we
fall back from en-GB to en-US (e.g., we don't fall back from pt-BR to pt-PT).

http://codereview.adblockplus.org/11165026/diff/1/lib/ui.js
File lib/ui.js (right):

http://codereview.adblockplus.org/11165026/diff/1/lib/ui.js#newcode425
lib/ui.js:425: let timer =
Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);
On 2013/07/19 13:53:52, Felix H. Dahlke wrote:
> I wasn't sure if we have to cancel this timer under certain circumstances, do
> we?

Sure, we do:

  onShutdown.add(function() timer.cancel());

But there is more: this timer will likely be cancelled automatically because the
corresponding object is garbage collected. We need to keep a reference to that
timer.

http://codereview.adblockplus.org/11165026/diff/1/lib/ui.js#newcode1935
lib/ui.js:1935: return;
We already marked the notification as shown at this point. It would make sense
to check the prerequisites first and only call Notification.getNextToShow()
then.

http://codereview.adblockplus.org/11165026/diff/1/lib/ui.js#newcode1947
lib/ui.js:1947: messageElement.innerHTML = message;
Ouch, big security vulnerability here - you are inserting HTML code received
from a web server (untrusted, might be hacked) into privileged context. That
HTML code could run JS code that will take over the computer.

Please have a look at chrome/content/ui/i18n.js on how to process "HTML" code
that is only allowed to have <a> and <strong> in it. Feel free to use <xul:label
class="text-link"> instead of <a>, then you won't have to style the link
yourself.

http://codereview.adblockplus.org/11165026/diff/1/lib/ui.js#newcode1958
lib/ui.js:1958: ui.loadInBrowser(url, window);
Please do /^https?:\/\//.test(url), otherwise you might introduce a security
vulnerability.

[Felix Dahlke, 2013-07-19 17:07:01]

Uploaded a new patch set addressing all the issues.

http://codereview.adblockplus.org/11165026/diff/1/lib/notification.js
File lib/notification.js (right):

http://codereview.adblockplus.org/11165026/diff/1/lib/notification.js#newcode54
lib/notification.js:54: let defaultLocale = "en";
On 2013/07/19 15:17:52, Wladimir Palant wrote:
> Actually, our default locale is always en-US - and that's the only reason why
we
> fall back from en-GB to en-US (e.g., we don't fall back from pt-BR to pt-PT).

Done.

http://codereview.adblockplus.org/11165026/diff/1/lib/ui.js
File lib/ui.js (right):

http://codereview.adblockplus.org/11165026/diff/1/lib/ui.js#newcode425
lib/ui.js:425: let timer =
Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);
On 2013/07/19 15:17:52, Wladimir Palant wrote:
> On 2013/07/19 13:53:52, Felix H. Dahlke wrote:
> > I wasn't sure if we have to cancel this timer under certain circumstances,
do
> > we?
> 
> Sure, we do:
> 
>   onShutdown.add(function() timer.cancel());
> 
> But there is more: this timer will likely be cancelled automatically because
the
> corresponding object is garbage collected. We need to keep a reference to that
> timer.

Done.

http://codereview.adblockplus.org/11165026/diff/1/lib/ui.js#newcode1935
lib/ui.js:1935: return;
On 2013/07/19 15:17:52, Wladimir Palant wrote:
> We already marked the notification as shown at this point. It would make sense
> to check the prerequisites first and only call Notification.getNextToShow()
> then.

Done.

http://codereview.adblockplus.org/11165026/diff/1/lib/ui.js#newcode1947
lib/ui.js:1947: messageElement.innerHTML = message;
On 2013/07/19 15:17:52, Wladimir Palant wrote:
> Ouch, big security vulnerability here - you are inserting HTML code received
> from a web server (untrusted, might be hacked) into privileged context. That
> HTML code could run JS code that will take over the computer.
> 
> Please have a look at chrome/content/ui/i18n.js on how to process "HTML" code
> that is only allowed to have <a> and <strong> in it. Feel free to use
<xul:label
> class="text-link"> instead of <a>, then you won't have to style the link
> yourself.

Done, completely forgot that this is an issue in Firefox, in Chrome it isn't.
Should be robust now.

As discussed, I'm sticking with HTML to ensure the text wraps properly.

http://codereview.adblockplus.org/11165026/diff/1/lib/ui.js#newcode1958
lib/ui.js:1958: ui.loadInBrowser(url, window);
On 2013/07/19 15:17:52, Wladimir Palant wrote:
> Please do /^https?:\/\//.test(url), otherwise you might introduce a security
> vulnerability.

Done.

[Wladimir Palant, 2013-07-20 07:18:07]

LGTM with the comments addressed.

http://codereview.adblockplus.org/11165026/diff/6/lib/ui.js
File lib/ui.js (right):

http://codereview.adblockplus.org/11165026/diff/6/lib/ui.js#newcode434
lib/ui.js:434: onShutdown.add(function() notificationTimer.cancel());
We need to verify that this doesn't cause an error if the timer already fired.

http://codereview.adblockplus.org/11165026/diff/6/lib/ui.js#newcode1998
lib/ui.js:1998: }
The URL no longer comes from server so we can just assume that it is safe - this
code isn't required now.

[Felix Dahlke, 2013-07-20 23:00:51]

Pushed my changes, since I think they're fine now. Do note my comment about
cancelling the timer below though, might have to be revisited.

http://codereview.adblockplus.org/11165026/diff/6/lib/ui.js
File lib/ui.js (right):

http://codereview.adblockplus.org/11165026/diff/6/lib/ui.js#newcode434
lib/ui.js:434: onShutdown.add(function() notificationTimer.cancel());
On 2013/07/20 07:18:07, Wladimir Palant wrote:
> We need to verify that this doesn't cause an error if the timer already fired.

I haven't see us do that in other places, so it seems to be fine, does it? The
docs don't say anything about whether cancelling an already fired one shot timer
should be a problem, nor does it seem to be possible to figure out if a timer
has already been fired. So I suppose I'd have to manually remove the shutdown
handler then.

Just tested this by closing Firefox after the notification has been shown. No
errors or anything, so it seems to be fine.

[Wladimir Palant, 2013-07-21 11:30:17]

http://codereview.adblockplus.org/11165026/diff/6/lib/ui.js
File lib/ui.js (right):

http://codereview.adblockplus.org/11165026/diff/6/lib/ui.js#newcode434
lib/ui.js:434: onShutdown.add(function() notificationTimer.cancel());
On 2013/07/20 23:00:51, Felix H. Dahlke wrote:
> I haven't see us do that in other places, so it
> seems to be fine, does it?

I other places we are using repeating timers, not one-time timers.

> Just tested this by closing Firefox after the notification has been shown. No
> errors or anything, so it seems to be fine.

The way to test this would be to disable Adblock Plus - it's extension shutdown,
not the browser shutdown ;)

Either way, I verified that nsITimer.cancel() doesn't throw errors even when
called multiple times.

[Felix Dahlke, 2013-07-21 12:20:28]

On 2013/07/21 11:30:17, Wladimir Palant wrote:
> http://codereview.adblockplus.org/11165026/diff/6/lib/ui.js
> File lib/ui.js (right):
> 
> http://codereview.adblockplus.org/11165026/diff/6/lib/ui.js#newcode434
> lib/ui.js:434: onShutdown.add(function() notificationTimer.cancel());
> On 2013/07/20 23:00:51, Felix H. Dahlke wrote:
> > I haven't see us do that in other places, so it
> > seems to be fine, does it?
> 
> I other places we are using repeating timers, not one-time timers.
> 
> > Just tested this by closing Firefox after the notification has been shown.
No
> > errors or anything, so it seems to be fine.
> 
> The way to test this would be to disable Adblock Plus - it's extension
shutdown,
> not the browser shutdown ;)
> 
> Either way, I verified that nsITimer.cancel() doesn't throw errors even when
> called multiple times.

Oh, I thought it was browser shutdown :P Well thanks for verifying this, closing
the review then.