463270 - ###!!! ASSERTION: Must have a principal: 'mOwner'

Status: RESOLVED FIXED

(Firefox :: General, defect, P2)

Description

[Carsten Book [:Tomcat]]

Attachment: stack

Found during the Automated Global 500 Test using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b2pre) Gecko/20081030
Minefield/3.1b2pre on http://shopping.uol.com.br/?&rtrk=src:13;size:18;chn:103;creative:link_fixo;thm:barra_navegacao1

###!!! ASSERTION: Must have a principal: 'mOwner', file c:/work/mozilla/builds/1.9.1-trace-malloc/mozilla/content/html/document/src/nsWyciwygChannel.cpp, line 293

Comment 1

[Boris Zbarsky [:bzbarsky]]

Can't reproduce by loading the page...  :(

Comment 2

[Jesse Ruderman]

tomcat, still seeing this?

Comment 3

[Mats Palmgren (inactive)]

Mochitest docshell/test/navigation/test_bug270414.html triggers this
assertion in Firefox trunk on Linux.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Aha.  The problem there is that our UI decides to load "wyciwyg:/favicon.ico" as a favicon.

Notes based on IRC conversation with gavin:

1)  It might be worth only doing this for URIs that are nsIURL, since appending
    "/favicon.ico" to prePath makes no sense to other URIs.
2)  It might be a good idea to pass the URI we plan to use as the base for
    prePath to shouldLoadFavIcon instead of passing a different URI object.
3)  It might make sense to get the exposable URI of whatever URI we're planning
    to use before doing anything else with it (so that favicon matches url bar).

Comment 5

[Mike Beltzner [:beltzner, not reading bugmail]]

--> Core::Content

Comment 6

[Mike Beltzner [:beltzner, not reading bugmail]]

Restoring flag.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Uh, no.  This really is a Firefox bug; see comment 4.  The UI code is doing things it's not supposed to.  It needs to stop doing that.

Comment 8

[Boris Zbarsky [:bzbarsky]]

And to be even more clear, all three items listed in comment 4 are issues in the UI code.

Comment 9

[Mike Beltzner [:beltzner, not reading bugmail]]

Whoops, thanks, Boris. I take it that you think this is a significant enough problem on which to block a release, yes? There's no indication of whether or not this is a regression or just something we'd never noticed until now, sadly.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Well....  I think it's a pretty serious issue that should be fixed, not least because it interferes with debugging.

Also, items 2 and 3 in the list above _may_ cause security issues in some edge cases (e.g. some extension-implemented URI schemes).  I haven't gone through a detailed analysis to make sure.

If push comes to shove, we could probably ship without fixing this, I guess...

I'm pretty sure this has always been a problem.

Comment 11

[David Dahl :ddahl]

Should bug 461010 block this bug?

Comment 12

[David Dahl :ddahl]

Attempted STR, and cannot get the assertion. Tried on latest trunk windows and linux. Also ran all unit tests in docshell/test/ - 3 tests do fail, but not the one mentioned in comment 3.

Comment 13

[Boris Zbarsky [:bzbarsky]]

David, the test in question doesn't _fail_.  It asserts.

If you want a simple way to reproduce, load any webpage (this bug page will do) and type:

  javascript:document.write("aaa"); document.close(); void(0);

in the url bar.  Then hit enter.  Observe the resulting assertion.

Comment 14

[David Dahl :ddahl]

Ah,I see. I did not see any kind of assertion in the test runs I did yesterday. I would seem this assertion is raised when loading any resource into the browser, not just through how the favicon is fetched and rendered.

Via bz's STR:

###!!! ASSERTION: XPConnect is being called on a scope without a 'Components' property!: 'Error', file c:/moz/mozilla-ce
ntral/mozilla/js/src/xpconnect/src/xpcwrappednativescope.cpp, line 786

Comment 15

[Boris Zbarsky [:bzbarsky]]

David, when I do my steps to reproduce I see the assertion this bug is filed on.  The assertion you list in comment 14 has nothing to do with this bug.

Comment 16

[David Dahl :ddahl]

I did a clobber build and now get the same thing, this is what is echoed when I follow comment 13 STR:

++DOMWINDOW == 15 (0xad983fb0) [serial = 19] [outer = 0xb6be4e40]
WARNING: NS_ENSURE_TRUE(childNum >= 0) failed: file /home/ddahl/code/moz/mozilla-central/mozilla/content/base/src/nsTreeWalker.cpp, line 416
WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x8000FFFF: file /home/ddahl/code/moz/mozilla-central/mozilla/content/base/src/nsTreeWalker.cpp, line 281
WARNING: NS_ENSURE_TRUE(childNum >= 0) failed: file /home/ddahl/code/moz/mozilla-central/mozilla/content/base/src/nsTreeWalker.cpp, line 416
WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x8000FFFF: file /home/ddahl/code/moz/mozilla-central/mozilla/content/base/src/nsTreeWalker.cpp, line 281
###!!! ASSERTION: XPConnect is being called on a scope without a 'Components' property!: 'Error', file /home/ddahl/code/moz/mozilla-central/mozilla/js/src/xpconnect/src/xpcwrappednativescope.cpp, line 786
###!!! ASSERTION: Must have a principal: 'mOwner', file /home/ddahl/code/moz/mozilla-central/mozilla/content/html/document/src/nsWyciwygChannel.cpp, line 293
WARNING: NS_ENSURE_TRUE(mOwner) failed: file /home/ddahl/code/moz/mozilla-central/mozilla/content/html/document/src/nsWyciwygChannel.cpp, line 298
###!!! ASSERTION: Must have a principal: 'mOwner', file /home/ddahl/code/moz/mozilla-central/mozilla/content/html/document/src/nsWyciwygChannel.cpp, line 293
WARNING: NS_ENSURE_TRUE(mOwner) failed: file /home/ddahl/code/moz/mozilla-central/mozilla/content/html/document/src/nsWyciwygChannel.cpp, line 298


The XPConnect assertion might be bug 426554 or bug 425298

Comment 17

[David Dahl :ddahl]

Attachment: WIP: assertion goes away, but...

sid and I went through the stack into tabbrowser.xml. This patch uses browser.contentDocument.documentURIObject.prePath instead of browser.currentURI.prePath for the favicon url. 

No assertions are raised, but surely this one liner is not the solution? is there another reason for using browser.currentURI.prePath instead of browser.contentDocument.documentURIObject.prePath?

Comment 18

[:Gavin Sharp [email: [e-mail]]]

That patch implements option 2) from bz's list in comment 4, which looks fine (minus the dump()s).

I think we should also implement 1), and only attempt to use prePath+"favicon.ico" if (documentURIObject instanceof Ci.nsIURL). Sound reasonable?

Comment 19

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 401140 [details] [diff] [review]
WIP: assertion goes away, but...

Oh, and the reason they're different is that bug 453442 only changed one of them. No good reason for that, really.

I also just realized that there's no need for the additional instanceof nsIURL check, since shouldLoadFavicon already checks for http[s] specifically, which is more restrictive.

So, that patch looks fine, minus the dumps. I'd probably also refactor to use a local variable to hold documentURIObject, rather than getting it twice. I'll r+ a patch with those changes.

Comment 20

[David Dahl :ddahl]

Attachment: v 1.0 Patch

Gavin: not sure if this test is ok, will an ASSERTION throw an exception normally in browser chrome tests?

Comment 21

[:Gavin Sharp [email: [e-mail]]]

Looks like you forgot to hg add the test.

I don't think we have any debug unit tests at the moment. I wouldn't bother with a test for this specifically, really, though a test that makes sure our favicon.ico fallback works correctly would be useful. I don't know whether one already exists.

Comment 22

[David Dahl :ddahl]

Attachment: v 1.1 Patch

Sorry - I added the test - if we don't need it I will remove it.

Comment 23

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 401260 [details] [diff] [review]
v 1.1 Patch

r=me with the test removed (doesn't currently test anything as-is)

Comment 24

[David Dahl :ddahl]

Attachment: v 1.2 Trunk Patch

Removed the test - I thought that the test might fail if an assertion was caught, which would throw an exception.

Comment 25

[David Dahl :ddahl]

Just tested this patch on 1.9.2 branch, you can use the "Trunk Patch" for both trunk and branch.

Comment 26

[:Gavin Sharp [email: [e-mail]]]

https://hg.mozilla.org/mozilla-central/rev/65bfeea3c2d7
https://hg.mozilla.org/releases/mozilla-1.9.2/rev/72a96c9dccc9