Last Comment Bug 813801 - null-check blocklistWindow in nsBlocklistService.js
: null-check blocklistWindow in nsBlocklistService.js
Status: RESOLVED FIXED
[good first bug][mentor=bmcbride@mozi...
:
Product: Toolkit
Classification: Components
Component: Add-ons Manager (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla20
Assigned To: zeyu [:car]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-11-20 16:26 PST by Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not)
Modified: 2012-12-06 02:20 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
add a null check (1.26 KB, patch)
2012-11-24 02:15 PST, zeyu [:car]
dtownsend: review+
Details | Diff | Review

Description Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not) 2012-11-20 16:26:30 PST
It should only be possible to hit this in xpcshell tests, but it'd be nice to at least not have this littering the logs:

*** WARN addons.manager: Exception calling callback: TypeError: blocklistWindow is null


This happens because the xpcshell tests can't open windows, so the open() call returns null. 

Just need a null check in there:
https://hg.mozilla.org/mozilla-central/file/f42a83257653/toolkit/mozapps/extensions/nsBlocklistService.js#l1000


No test needed, though bonus points if you manually verify that the above warning shops showing when running the following xpcshell test:
toolkit/mozapps/extensions/test/xpcshell/test_blocklistchange.js
Comment 1 zeyu [:car] 2012-11-24 02:15:42 PST
Created attachment 684837 [details] [diff] [review]
add a null check

I didn't test, can you please guide me how to run test? Thanks!
Comment 2 Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not) 2012-11-25 14:25:07 PST
Great :) I'll give you a chance to test it before I review your patch.


(In reply to zeyu [:car] from comment #1)
> I didn't test, can you please guide me how to run test? Thanks!

You can run just that test via running the following in your repository:

  mach xpcshell-test toolkit/mozapps/extensions/test/xpcshell/test_blocklistchange.js

Or if you remove just the filename, keeping the path, it'll run all xpcshell tests in that directory.

The following page has all the gritty details, though it describes the old (more complex) way of running this type of test:
https://developer.mozilla.org/en-US/docs/Writing_xpcshell-based_unit_tests
Comment 3 zeyu [:car] 2012-11-29 05:08:01 PST
Hi Blair,

I tried to run the test, however it seems that the number of lines that can be displayed in terminal is limited so that I cannot see the full log. I also tried 

./mach -l log.log xpcshell-test toolkit/mozapps/extensions/test/xpcshell /test_blocklistchange.js

it only gave me an empty file. So how can I see the full test log? Thanks! BTW, I am running Ubuntu 12.04
Comment 4 Gregory Szorc [:gps] 2012-12-03 13:38:58 PST
Mach doesn't currently log test output. I suppose we should file a bug on that.

You can always do something like:

./mach xpcshell-test path/to/test > log.log
Comment 5 zeyu [:car] 2012-12-03 19:40:04 PST
I've run the test and I believe the warning was gone.
Comment 6 Dave Townsend [:mossop] 2012-12-05 13:07:58 PST
Comment on attachment 684837 [details] [diff] [review]
add a null check

Taking this since Blair is out sick today
Comment 7 Dave Townsend [:mossop] 2012-12-05 14:15:19 PST
Comment on attachment 684837 [details] [diff] [review]
add a null check

Looks good. One minor style nit, the rest of this file doesn't use braces around single statements like this. I've gone ahead and corrected that and will land it in a moment.

Thanks for the patch!
Comment 8 Dave Townsend [:mossop] 2012-12-05 14:15:50 PST
Landed on inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/c615b961c7b4
Comment 9 Ed Morley [:emorley] 2012-12-06 02:20:01 PST
https://hg.mozilla.org/mozilla-central/rev/c615b961c7b4

Note You need to log in before you can comment on or make changes to this bug.