1130816 - Bad argument passed to newChannelFromURI2 in WindowsPreviewPerTab.jsm

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Blair McBride [:Unfocused] (UNAVAILABLE)]

I'm getting this when running tests:

JavaScript error: resource:///modules/WindowsPreviewPerTab.jsm, line 77: NS_ERROR_XPC_BAD_CONVERT_JS: Could not convert JavaScript argument arg 1 [nsIIOService2.newChannelFromURI2]
3 INFO Console message: [JavaScript Error: "NS_ERROR_XPC_BAD_CONVERT_JS: Could not convert JavaScript argument arg 1 [nsIIOService2.newChannelFromURI2]" {file: "resource:///modules/WindowsPreviewPerTab.jsm" line: 77}]
JavaScript error: resource:///modules/WindowsPreviewPerTab.jsm, line 77: NS_ERROR_XPC_BAD_CONVERT_JS: Could not convert JavaScript argument arg 1 [nsIIOService2.newChannelFromURI2]
4 INFO Console message: [JavaScript Error: "NS_ERROR_XPC_BAD_CONVERT_JS: Could not convert JavaScript argument arg 1 [nsIIOService2.newChannelFromURI2]" {file: "resource:///modules/WindowsPreviewPerTab.jsm" line: 77}]


I assume this is the result of bug 1087726.

Comment 1

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Christoph, could you take a look at this?

Comment 2

[Philip Chee]

I think the problem is here:
https://hg.mozilla.org/mozilla-central/rev/93c1130d0d47#l3.40
The inner call to _imageFromURI has the wrong number of arguments. Actually this was already broken before Bug 1087726, now it's just more broken.

Comment 3

[:Gijs (out for now; he/him)]

(In reply to Philip Chee from comment #2)
> I think the problem is here:
> https://hg.mozilla.org/mozilla-central/rev/93c1130d0d47#l3.40
> The inner call to _imageFromURI has the wrong number of arguments. Actually
> this was already broken before Bug 1087726, now it's just more broken.

Eh? It had 3 arguments, now has 4, and that seems to be what's being passed...

Seems to me (without having run this) that it's more likely that in tests the contentWindow.document ref is null and so we're passing null as the owning document, which newChannelFromURI2 doesn't like... in any case, it should be easy to test either of these hypotheses with the JS debugger ( --jsdebugger) and a conditional breakpoint in the relevant code.

Comment 4

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1130816.patch

Either of your hypotheses regarding the incorrect first argument could be correct:
* either the doc is null, or
* we can not convert a nsIURI into an nsINode.

The second one seems more likely to me - the attached patch should fix the problem. Also, even before we changed the arguments of _imageFromURI, the one callsite was missing the 'prviateMode' argument - included that in the patch as well.

If that attached patch fixed the problem please flag gijs for review. Otherwise please let me know what testcase you are running so I can reproduce on my end.

Thanks for filing the bug!

Comment 5

[Christoph Kerschbaumer [:ckerschb]]

Oh, also clearing my needinfo flag!

Comment 6

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8561200 [details] [diff] [review]
bug_1130816.patch

Review of attachment 8561200 [details] [diff] [review]:
-----------------------------------------------------------------

Yea, this seems to fix it - and the fix looks correct to me.

::: browser/modules/WindowsPreviewPerTab.jsm
@@ +97,5 @@
>        // We failed, so use the default favicon (only if this wasn't the default
>        // favicon).
>        let defaultURI = faviconSvc.defaultFavicon;
>        if (!defaultURI.equals(uri))
> +        _imageFromURI(doc, defaultURI, privateMode, callback);

This looks like another bug: callback isn't guaranteed to be called.

Comment 7

[Christoph Kerschbaumer [:ckerschb]]

Great, still would like Gijs have a quick look since he reviewed the main patch.

Comment 8

[:Gijs (out for now; he/him)]

Comment on attachment 8561200 [details] [diff] [review]
bug_1130816.patch

Review of attachment 8561200 [details] [diff] [review]:
-----------------------------------------------------------------

Ugh. Can't believe I missed this. :-(

Comment 9

[:Gijs (out for now; he/him)]

(In reply to Blair McBride [:Unfocused] (I don't read bugmail - needinfo? me!) from comment #6)
> ::: browser/modules/WindowsPreviewPerTab.jsm
> @@ +97,5 @@
> >        // We failed, so use the default favicon (only if this wasn't the default
> >        // favicon).
> >        let defaultURI = faviconSvc.defaultFavicon;
> >        if (!defaultURI.equals(uri))
> > +        _imageFromURI(doc, defaultURI, privateMode, callback);
> 
> This looks like another bug: callback isn't guaranteed to be called.

Only in two cases, though, right, unless I'm missing something else (it's been known to happen!) -

1) the resultcode of the fetch != NS_OK, in which case we just bail and don't call the callback. Considering the callbacks here only update the icon if it's non-empty, leaving it empty seems... OK?
2) if fetching the icon for the default favicon URI failed. Obviously, that shouldn't happen... but if it did, I guess leaving the icon un-set would also probably be the sensible approach, rather than just trying again?

Comment 10

[Christoph Kerschbaumer [:ckerschb]]

(In reply to :Gijs Kruitbosch from comment #9)
> (In reply to Blair McBride [:Unfocused] (I don't read bugmail - needinfo?
> me!) from comment #6)
> > ::: browser/modules/WindowsPreviewPerTab.jsm
> > @@ +97,5 @@
> > >        // We failed, so use the default favicon (only if this wasn't the default
> > >        // favicon).
> > >        let defaultURI = faviconSvc.defaultFavicon;
> > >        if (!defaultURI.equals(uri))
> > > +        _imageFromURI(doc, defaultURI, privateMode, callback);
> > 
> > This looks like another bug: callback isn't guaranteed to be called.
> 
> Only in two cases, though, right, unless I'm missing something else (it's
> been known to happen!) -
> 
> 1) the resultcode of the fetch != NS_OK, in which case we just bail and
> don't call the callback. Considering the callbacks here only update the icon
> if it's non-empty, leaving it empty seems... OK?
> 2) if fetching the icon for the default favicon URI failed. Obviously, that
> shouldn't happen... but if it did, I guess leaving the icon un-set would
> also probably be the sensible approach, rather than just trying again?

Thanks Gijs for the explanation and the speedy review, but that's going to be a different bug.

I am landing the attached patch as it fixes the problem described in this bug:
  https://hg.mozilla.org/integration/mozilla-inbound/rev/b7e806aa4fe8

Comment 11

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to :Gijs Kruitbosch from comment #9)
> 1) the resultcode of the fetch != NS_OK, in which case we just bail and
> don't call the callback. Considering the callbacks here only update the icon
> if it's non-empty, leaving it empty seems... OK?
> 2) if fetching the icon for the default favicon URI failed. Obviously, that
> shouldn't happen... but if it did, I guess leaving the icon un-set would
> also probably be the sensible approach, rather than just trying again?

Yea, I don't think those are particularly bad. I guess there's the question of whether a stale image or no image is a better fail-mode.

But what worries me is that the code isn't being intentional about it. That's going to bite us on our collective asses in the future.

Comment 12

[:Gijs (out for now; he/him)]

(In reply to Blair McBride [:Unfocused] (I don't read bugmail - needinfo? me!) from comment #11)
> (In reply to :Gijs Kruitbosch from comment #9)
> > 1) the resultcode of the fetch != NS_OK, in which case we just bail and
> > don't call the callback. Considering the callbacks here only update the icon
> > if it's non-empty, leaving it empty seems... OK?
> > 2) if fetching the icon for the default favicon URI failed. Obviously, that
> > shouldn't happen... but if it did, I guess leaving the icon un-set would
> > also probably be the sensible approach, rather than just trying again?
> 
> Yea, I don't think those are particularly bad. I guess there's the question
> of whether a stale image or no image is a better fail-mode.
> 
> But what worries me is that the code isn't being intentional about it.
> That's going to bite us on our collective asses in the future.

Fair. Want to file a followup to ensure we always call it (probably with |null|) and deal with that in the relevant callbacks? :-)

Comment 13

[Blair McBride [:Unfocused] (UNAVAILABLE)]

And here I was trying my best to be lazy. Filed bug 1131428.

Comment 14

[Legacy TBPL/Treeherder Robot]

log: https://treeherder.mozilla.org/logviewer.html#?repo=try&job_id=4829602
repository: try
start_time: 2015-02-10T01:57:46
who: mgoodwin[at]mozilla[dot]com
machine: t-w732-ix-050
buildname: Windows 7 32-bit try debug test mochitest-devtools-chrome-3
revision: 3e88d62a7bf4

696 INFO TEST-UNEXPECTED-FAIL | browser/devtools/webconsole/test/browser_webconsole_certificate_messages.js | Timed out while waiting for: SSL3 warning displayed successfully -
697 INFO TEST-UNEXPECTED-FAIL | browser/devtools/webconsole/test/browser_webconsole_certificate_messages.js | A promise chain failed to handle a rejection:  - null
698 INFO TEST-UNEXPECTED-FAIL | browser/devtools/webconsole/test/browser_webconsole_certificate_messages.js | Test timed out - expected PASS
Return code: 1

Comment 15

[Legacy TBPL/Treeherder Robot]

log: https://treeherder.mozilla.org/logviewer.html#?repo=try&job_id=4829603
repository: try
start_time: 2015-02-10T01:57:55
who: mgoodwin[at]mozilla[dot]com
machine: t-w732-ix-104
buildname: Windows 7 32-bit try debug test mochitest-devtools-chrome-3
revision: 3e88d62a7bf4

696 INFO TEST-UNEXPECTED-FAIL | browser/devtools/webconsole/test/browser_webconsole_certificate_messages.js | Timed out while waiting for: SSL3 warning displayed successfully -
697 INFO TEST-UNEXPECTED-FAIL | browser/devtools/webconsole/test/browser_webconsole_certificate_messages.js | A promise chain failed to handle a rejection:  - null
698 INFO TEST-UNEXPECTED-FAIL | browser/devtools/webconsole/test/browser_webconsole_certificate_messages.js | Test timed out - expected PASS
Return code: 1

Comment 16

[Ryan VanderMeulen [:RyanVM]]

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