791652 - Navigator::GetMozTime doesn't always set its outparam when returning success

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Comment 1

[Justin Lebar (not reading bugmail)]

Oops.

Steven, XPCOM rules are that if you return NS_OK, you must first set the outparam to something.

Navigator::GetMozTime has a bunch of early returns where we return NS_OK, but we need to first set *aTime to nullptr.

Comment 2

[StevenLee[:slee]]

Attachment: patch

Comment 3

[StevenLee[:slee]]

Hi Justin,
I found that some getter functions that need permission in Navigator.cpp may have
problems. For example, http://mxr.mozilla.org/mozilla-central/source/dom/base/Navigator.cpp#1205, in the first time, it does permission check. But in the 
following gettings, the checking code will not be ran, is that right?

Comment 4

[Justin Lebar (not reading bugmail)]

Comment on attachment 661811 [details] [diff] [review]
patch

Should I push this and the patch in bug 791682 for you?

Comment 5

[StevenLee[:slee]]

(In reply to Justin Lebar [:jlebar] from comment #4)
> Comment on attachment 661811 [details] [diff] [review]
> patch
> 
> Should I push this and the patch in bug 791682 for you?

OK, please help me push these patches.
Thanks.

Comment 6

[Justin Lebar (not reading bugmail)]

> But in the following gettings, the checking code will not be ran, is that right?

I think it's OK because each inner window gets a new navigator.  See nsGlobalWindow::GetNavigator().

It looks like the navigator object is re-used in some edge cases.  See the calls to mNavigator->OnNavigation() and to mNavigator->SetWindow() in nsGlobalWindow.

I think the SetWindow() call is safe because the code claims the new window should have the same origin as the old window.  I'm not entirely clear on what OnNavigation() is for, but it's not modifying the window, so I guess that's OK too.

So, good question, but I think we're in the clear.

Comment 7

[StevenLee[:slee]]

(In reply to Justin Lebar [:jlebar] from comment #6)
> I think the SetWindow() call is safe because the code claims the new window
> should have the same origin as the old window.  I'm not entirely clear on
> what OnNavigation() is for, but it's not modifying the window, so I guess
> that's OK too.
> 
> So, good question, but I think we're in the clear.
Thanks for your explanations.
I know the code better. :)

Comment 8

[Justin Lebar (not reading bugmail)]

> Bug791652 Navigator::GetMozTime doesn't always set its outparam when returning success, r=jlebar

btw, in the future, please write the beginning of the commit message as

  "Bug 791652: Navigator::GetMozTime ..."

or

  "Bug 791652 - Navigator ...".

People often grep our logs for "Bug XXX" and "BugXXX" is an uncommon form.

It's also customary to try to describe your fix, rather than the bug, in the commit message.  That means your commit message won't always match the title of the bug, and that's fine.  In this case, we might say something like "Set outparam when returning success from Navigator::GetMozTime".

Anyway, thanks for your quick patch.  :)

https://hg.mozilla.org/integration/mozilla-inbound/rev/c5e3744772c4

Comment 9

[Graeme McCutcheon [:graememcc]]

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