1539997 - NS_ERROR_MALFORMED_URI errors seen during |make mozmill| test of FULL DEBUG version of TB

Status: NEW

(MailNews Core :: Backend, defect)

Description

[ISHIKAWA, Chiaki (back from day job exhibition event.)]

Attachment: Patch to nsIURIMutator.idl to print |aSpec| value when SetSpec() failed.

After updating the source files on late last week, now I get
the following errors:

From my summary listing of |make mozmill| by running FULL DEBUG version of
C-C TB.

155: Main Thread] WARNING: NS_ENSURE_SUCCESS(mStatus, *this) failed with
result 0x804B000A: file
/KERNEL-SRC/moz-obj-dir/objdir-tb3/dist/include/nsIURIMutator.h, line 472
152: Main Thread] WARNING: NS_ENSURE_SUCCESS(mStatus, mStatus) failed with
result 0x804B000A: file
/KERNEL-SRC/moz-obj-dir/objdir-tb3/dist/include/nsIURIMutator.h, line 622

0x804B000A (2152398858) is NS_ERROR_MALFORMED_URI.

For example, I see the errors in test_persist_tabs:

TEST-START |
/NREF-COMM-CENTRAL/mozilla/comm/mail/test/mozmill/pref-window/test-attachments-pane.js
| test_persist_tabs
++DOCSHELL 0x5647cc18c9d0 == 12 [pid = 3994] [id =
{860cf41c-5922-4895-bb57-84488fd948fe}]
++DOMWINDOW == 29 (0x5647cbbb9a80) [pid = 3994] [serial = 29] [outer = (nil)]
++DOMWINDOW == 30 (0x5647cb3b4710) [pid = 3994] [serial = 30] [outer =
0x5647cbbb9a80]
++DOMWINDOW == 31 (0x5647ccabe820) [pid = 3994] [serial = 31] [outer =
0x5647cbbb9a80]
[3994, Main Thread] WARNING: NS_ENSURE_SUCCESS(mStatus, *this) failed with
result 0x804B000A: file
/KERNEL-SRC/moz-obj-dir/objdir-tb3/dist/include/nsIURIMutator.h, line 472
[3994, Main Thread] WARNING: NS_ENSURE_SUCCESS(mStatus, mStatus) failed with
result 0x804B000A: file
/KERNEL-SRC/moz-obj-dir/objdir-tb3/dist/include/nsIURIMutator.h, line 622
[3994, Main Thread] WARNING: NS_ENSURE_SUCCESS(mStatus, *this) failed with
result 0x804B000A: file
/KERNEL-SRC/moz-obj-dir/objdir-tb3/dist/include/nsIURIMutator.h, line 472
[3994, Main Thread] WARNING: NS_ENSURE_SUCCESS(mStatus, mStatus) failed with
result 0x804B000A: file
/KERNEL-SRC/moz-obj-dir/objdir-tb3/dist/include/nsIURIMutator.h, line 622

... similar pair of lines ...

Now, I dumped the value of aSpec that is passed to SetSpec function that returned error.

I found that the aSpec was of the following forms when the errors occurred.
(Number at the beginning of the line is the frequency of appearances.)

Now I figure we have incorrect URLs indeed.

Obviously, the proper URL form is not given (protocol and host parts part are missing).
In one case, the protocol part "moz-icon:" prefix probably is not recognized by M-C code.

 40 {debug} SetSpec failed : aSpec=branding/brand.ftl
 20 {debug} SetSpec failed : aSpec=messenger/aboutSupportMail.ftl
 16 {debug} SetSpec failed : aSpec=messenger/preferences/fonts.ftl
 16 {debug} SetSpec failed : aSpec=messenger/preferences/languages.ftl
 16 {debug} SetSpec failed : aSpec=messenger/preferences/preferences.ftl
  3 {debug} SetSpec failed : aSpec=moz-icon:http://www.mozilla.org/
  4 {debug} SetSpec failed : aSpec=toolkit/about/aboutAddons.ftl
 20 {debug} SetSpec failed : aSpec=toolkit/about/aboutSupport.ftl
 20 {debug} SetSpec failed : aSpec=toolkit/global/resetProfile.ftl 

TIA

I am attaching a patch that was used to dump the aSpec value after the failure.

Comment 1

[ISHIKAWA, Chiaki (back from day job exhibition event.)]

3 {debug} SetSpec failed : aSpec=moz-icon:http://www.mozilla.org/

Come to think of it, this spec has two prefixes sort of: the first one is "moz-icon" followed by another protocol prefix "http:".
Not sure what the proper "URI" is expected by M-C code here.

IMHO, we should document the usage of "moz-icon:" along with four (?) other application-oriented protocol-like prefix, "message:", etc.

Comment 2

[Wayne Mery (:wsmwk)]

ready for review?

Comment 3

[ISHIKAWA, Chiaki (back from day job exhibition event.)]

(In reply to Wayne Mery (:wsmwk) from comment #2)

ready for review?

I am not sure what we want to about this.

My take on this is that:

• there was a time when invalid URIs were passed as I mentioned in the original post.
  (I am not sure how it is. I suppose it still IS the case)

• A surprise: M-C seems to have REMOVED the CHECK (!?)
  Sure this REMOVES the error/warning in debug build.
  HOWEVER I think this is a BOGUS change, and frankly I want to know the reason behind such a change.
  There were/are places where invalid URIs were passed around and the code elsewhere
  either did not check or is written in a loose manner that it works by chance, but maybe it is not doing exactly
  what the original author intended.

Comment 4

[ISHIKAWA, Chiaki (back from day job exhibition event.)]

I am not sure how to clear the needinfo request. Does this work?

Comment 5

[Jorg K (CEST = GMT+2)]

Let's track it if we know what the issue is.

Comment 6

[ISHIKAWA, Chiaki (back from day job exhibition event.)]

Maybe I should have marked it as FF core problem :-(

I have found several bugzilla entries after searching for SetSpec.
The following two stood out.

bug 359901 nsSimpleURI::SetSpec misidentifies nsIURI::prePath and ::path for HTTP URLs
bug 91908 nsStdURL::SetSpec() Assertion when reading Messages with image/news attatchments

There is something fishy with this SetSpec() code.
I just noticed invalid URIs are passed before the error occurs in
Bug 1484449
TEST-UNEXPECTED-FAIL | /builds/worker/workspace/build/tests/mozmill/pref-window/test-font-chooser.js
but not sure if SetSpec() issues are related to Bug 1484449.

Comment 7

[ISHIKAWA, Chiaki (back from day job exhibition event.)]

I agree with bug 91908 comment 2
Quote

for whatever reason, the code is trying to open up a URI which does not have a
scheme. maybe the nsMsgMailNewsUrl needs to check for this case, and prepend a
known scheme.
End Quote

In the case of Bug 1484449,
we see
{debug} SetSpec failed : aSpec=branding/brand.ftl
{debug} SetSpec failed : aSpec=messenger/preferences/preferences.ftl
{debug} SetSpec failed : aSpec=messenger/preferences/fonts.ftl
{debug} SetSpec failed : aSpec=messenger/preferences/languages.ftl
which certainly seem to lack proper scheme to form a complete URI to me.
(I think the upper layer that calls SetSpec() needs to be fixed.)
M-C code change hid the problem under the carpet instead of fixing the bad caller sites. )