286702 - [FIXr]###!!! ASSERTION: Bad loading table: 'mLoadingDatas.Get(aLoadData->mURI, &loadingData) && loadingData == aLoadData', file r:/mozilla/layout/style/nsCSSLoader.cpp, line 1486

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P1)

Description

[timeless]

###!!! ASSERTION: Bad loading table: 'mLoadingDatas.Get(aLoadData->mURI,
&loadingData) && loadingData == aLoadData', file
r:/mozilla/layout/style/nsCSSLoader.cpp, line 1486

+
{,,necko.dll}((*(nsACString*)(&(*(nsCSubstring*)(&(*(nsStandardURL*){*}(((*aLoadData).mURI).mRawPtr)).mSpec))))).mData
0x011790e0 "file:///css/homepage_import_rc.css"	char *
+	aLoadData	0x035d4d88 {mRefCnt={mValue=0x00000003 }
_mOwningThread={mThread=0x003f5198 } mLoader=0x01aaf320
{mRefCnt={mValue=0x00000003 } _mOwningThread={mThread=0x003f5198 }
gParsers=0x019caa50 ...} ...}	SheetLoadData *
	aLoadData->mIsLoading	0x01 '␁'	unsigned char
+	loadingData	0x00000000 {mRefCnt={mValue=??? } _mOwningThread={mThread=??? }
mLoader=??? ...}	SheetLoadData *
-	mLoadingDatas	{...}	nsDataHashtable<nsURIHashKey,SheetLoadData *>
\-	nsBaseHashtable<nsURIHashKey,SheetLoadData *,SheetLoadData *>	{...}
nsBaseHashtable<nsURIHashKey,SheetLoadData *,SheetLoadData *>
 \-	nsTHashtable<nsBaseHashtableET<nsURIHashKey,SheetLoadData *> >
{mTable={ops=0x0269a654 sOps data=0x00000000 hashShift=0x001c ...} }
nsTHashtable<nsBaseHashtableET<nsURIHashKey,SheetLoadData *> >
  \-	mTable	{ops=0x0269a654 sOps data=0x00000000 hashShift=0x001c ...}	PLDHashTable
   |+	ops	0x0269a654 sOps	const PLDHashTableOps *
   |	data	0x00000000	void *
   |	hashShift	0x001c	short
   |	maxAlphaFrac	0xc0 'À'	unsigned char
   |	minAlphaFrac	0x40 '@'	unsigned char
   |	entrySize	0x0000000c	unsigned int
   |	entryCount	0x00000001	unsigned int
   |	removedCount	0x00000000	unsigned int
   |	generation	0x00000000	unsigned int
   \+	entryStore	0x0356f2d8 ""	char *
	aSucceeded	0x00000000	int

	aStatus	0x80520001	unsigned int /*NS_ERROR_FILE_UNRECOGNIZED_PATH*/

 	xpcom_core.dll!nsDebugImpl::Assertion(const char * aStr=0x025ae8f0, const char
* aExpr=0x025ae8a0, const char * aFile=0x025ae878, int aLine=0x000005ce)  Line
301	C++
 	xpcom_core.dll!nsDebug::Assertion(const char * aStr=0x025ae8f0, const char *
aExpr=0x025ae8a0, const char * aFile=0x025ae878, int aLine=0x000005ce)  Line 109	C++
>	gklayout.dll!CSSLoaderImpl::SheetComplete(SheetLoadData *
aLoadData=0x035d4d88, int aSucceeded=0x00000000)  Line 1486 + 0x41	C++
 	gklayout.dll!SheetLoadData::OnStreamComplete(nsIUnicharStreamLoader *
aLoader=0x0349ee88, nsISupports * aContext=0x00000000, unsigned int
aStatus=0x80520001, nsIUnicharInputStream * aDataStream=0x00000000)  Line 791	C++
 	xpcom_core.dll!XPTC_InvokeByIndex(nsISupports * that=0x035d4d88, unsigned int
methodIndex=0x00000004, unsigned int paramCount=0x00000004, nsXPTCVariant *
params=0x0349eef0)  Line 102	C++
 	xpcom_core.dll!EventHandler(PLEvent * self=0x034c6e70)  Line 563 + 0x29	C++
 	xpcom_core.dll!PL_HandleEvent(PLEvent * self=0x034c6e70)  Line 698 + 0xa	C
 	xpcom_core.dll!PL_ProcessPendingEvents(PLEventQueue * self=0x0114a280)  Line
633 + 0x9	C
 	xpcom_core.dll!_md_EventReceiverProc(HWND__ * hwnd=0x007d15be, unsigned int
uMsg=0x0000c1d6, unsigned int wParam=0x00000000, long lParam=0x0114a280)  Line
1435 + 0x9	C
 	user32.dll!_InternalCallWinProc@20()  + 0x28	
 	user32.dll!_UserCallWinProcCheckWow@32()  + 0xb7	
 	user32.dll!_DispatchMessageWorker@8()  + 0xdc	
 	user32.dll!_DispatchMessageA@4()  + 0xf	
 	mfc71d.dll!AfxInternalPumpMessage()  Line 188	C++
 	mfc71d.dll!CWinThread::PumpMessage()  Line 916	C++
 	mfc71d.dll!CWinThread::Run()  Line 637 + 0xb	C++
 	mfc71d.dll!CWinApp::Run()  Line 701	C++
 	mfc71d.dll!AfxWinMain(HINSTANCE__ * hInstance=0x00400000, HINSTANCE__ *
hPrevInstance=0x00000000, char * lpCmdLine=0x00142384, int nCmdShow=0x0000000a)
 Line 49 + 0xb	C++
 	mfcembed.exe!WinMain(HINSTANCE__ * hInstance=0x00400000, HINSTANCE__ *
hPrevInstance=0x00000000, char * lpCmdLine=0x00142384, int nCmdShow=0x0000000a)
 Line 25	C++
 	mfcembed.exe!WinMainCRTStartup()  Line 390 + 0x39	C
 	kernel32.dll!_BaseProcessStart@4()  + 0x23	

i was using file>open to load http://www.macromedia.com, windows automatically
downloads just the text/html file to a temporary internet files folder, but
doesn't do things like fix up paths, so something asked for
/css/homepage_import_rc.css as a stylesheet and got poorly reparented. not sure
what other special bits it took to do this.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Please let me know if this is reproducible and what the steps to reproduce are....

Comment 2

[timeless]

Attachment: www.macromedia[1].com bug 286704

Comment 3

[timeless]

this assert will trigger if the file is saved to disk as something like '0.xml'
or '0.html'.

i can't think of a way to trigger this testcase using bugzilla. i tried the
foopy protocol, and it didn't work :(, it triggered something which mrbkap says
is probably bug 243338 / bug 84582.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed patch

Darin, the problem is that when trying to convert the URI "file:///foo/bar"
(note lack of drive letter) to an nsIFile on Windows, we get an exception. 
This can happen in various other ways too, for resource: and file:// URIs.

So what this patch does is to compare the strings if EnsureFile fails in an
indentical way on both URIs...

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: Same, but compiling

Comment 6

[Darin Fisher]

Comment on attachment 178325 [details] [diff] [review]
Same, but compiling

>Index: netwerk/base/src/nsStandardURL.cpp

>+        if (rv != rv2) {
>+            if (NS_FAILED(rv))
>+                LOG(("nsStandardURL::Equals [this=%p spec=%s] failed to "
>+                     "ensure file",
>+                     this, mSpec.get()));
>+
>+            if (NS_FAILED(rv2))
>+                LOG(("nsStandardURL::Equals [other=%p spec=%s] other failed "
>+                     "to ensure file",
>                 other, other->mSpec.get()));
>+
>+            // Claim mismatch, but also propagate the error we got
>+            *result = PR_FALSE;
>+            return NS_FAILED(rv) ? rv : rv2;
>         }

The if checks and LOG statements should probably be wrapped in a #ifdef
PR_LOGGING since we don't #define FORCE_PR_LOG in this file.  Wouldn't
some sort of NS_WARNING be better than LOG in these cases?  Or, are there
legit cases where EnsureFile would fail?

Also, there's no reason to set *result if the method is going to throw
an exception.  Doing so is inconsistent with other exit points from the
method, so I think you shouldn't bother setting *result here.

Comment 7

[Boris Zbarsky [:bzbarsky]]

I was thinking about this some more, and I really feel that returning error for
"reasonable" failures in EnsureFile (eg invalid file path, pointing to
nonexistent resource mapping, etc) seems wrong.

Darin thinks we should avoid creating such URIs at all (throw from newURI).  I'm
not sure how we can do that, exactly, unless we plan to basically duplicate all
the checks all platform nsLocalFile impls do (and this is just for file:// URIs;
resource: URIs are a separate story altogether).

So what do people think is a reasonable course of action here?

Comment 8

[Benjamin Smedberg]

If the URI is really malformed, then sure we should throw at newURI. But for a
missing resource: mapping, I think we should go as long as possible (similarly
for a missing chrome package or a non-existent file: URI drive letter): the
resource mapping could be added in the future; we shouldn't fail until
absolutely necessary.

Unless I'm misunderstanding the question.

Comment 9

[Boris Zbarsky [:bzbarsky]]

The question is twofold:

1)  Should we allow such URIs to exist?
2)  How should Equals() should for such URIs?  Right now it just throws, which is
    highly suboptimal in all cases where URIs are used as cache keys, since such
    a URI tests unequal to itself, basically.

Comment 10

[Darin Fisher]

It is easy to check that file:///home/foo is invalid under windows, so we should
perform that check in the URL parsers that get invoked as part of NewURI.

I guess that still leaves plenty of other interesting failure cases for EnsureFile.

But, at least for resource:// and chrome://, straight up string comparisons is
probably fine in the absence of actual files to compare.

So, I think we should just make sure that we aren't doing stupid things like
comparing file:///home/FOO/ to file:///home/foo/ and so on.

Comment 11

[Andreas Otte]

file:///home/foo is a valid url and that does not depend on the OS. It just does
not translate to a local file on windows. I see no reason to not create the url.
The URLParsers should not be bothered with this OS dependend stuff (it was a big
task to clean them up years ago), for those we have the converters to local
files.   

Comment 12

[Darin Fisher]

Well, we have a lot of other XP_WIN specific code in nsURLParsers.cpp:
http://lxr.mozilla.org/seamonkey/source/netwerk/base/src/nsURLParsers.cpp#404

Hmm... I suppose I could live with constructing nsIURIs from broken file:// URLs
on windows.  Anyways, we need to be smart in Equals to handle other cases where
EnsureFile returns failure, so we might as well lump this case in with those.  Ok.

Comment 13

[Boris Zbarsky [:bzbarsky]]

So perhaps we need two functions... EnsureFile(), and TryFile() (bad name, but
no better ideas so far).  The latter will be implemented by the same subclass
that implements EnsureFile and convert error codes that correspond to "not a
valid file" errors to NS_OK.

That said, if we do that we'll end up with file:///home/foo and file:///home/FOO
being compared as strings on Windows.  But I think that's reasonable...

Comment 14

[Boris Zbarsky [:bzbarsky]]

I'd really like to get some agreement on this and move forward... As things
stand, it's pretty easy to trigger leaks and/or crashes in any consumer that
uses an nsIURI as a hash key (and we have plenty of those).

Comment 15

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed patch

I think this patch is money.  It does the right thing for clearly identical
URIs, and does its best for the rest.

Comment 16

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 181314 [details] [diff] [review]
Proposed patch

+    // First, check whether both URIs are nsIFileURLs.

hm... that comments is confusing... what the if really checks is if one is a
file url and the other isn't. the nsIFiles are checked much later...

+ since conversion to file will
+	 // ignore the host!

maybe that should get a bug filed on?

+	 !SegmentIs(mPassword, other->mSpec.get(), other->mPassword) ||
+	 (Port() != other->Port());
+	 !SegmentIs(mParam, other->mSpec.get(), other->mParam)) {

uh... what's the semicolon in the port line doing here? (how does this
compile?)

Comment 17

[Boris Zbarsky [:bzbarsky]]

> uh... what's the semicolon in the port line doing here?

Hangin' out?  ;)  That in fact does not compile; I thought I'd checked, but I
checked in the wrong tree.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Attachment: Compiling and all

Comment 19

[Andreas Otte]

> 
> + since conversion to file will
> +	 // ignore the host!
> 
> maybe that should get a bug filed on?
> 

The host in a file url is somewhat redundant. It can only be localhost or the
local hostnamne, because the file protocol is by definition a local protocol.
Most of the time the hostname is already dropped from the url, but if it is
present it can savely be ignored because file://host1/path/file is the same as
file://host2/path/file on the local box. 

UNC-paths in file-urls are *paths* with network access, the host is still localhost.

Comment 20

[Boris Zbarsky [:bzbarsky]]

So two file URLs that point to the same file but have different hosts should
test equal?  Note that that's not what the patch implements, but it'd not be
hard to fix...

Comment 21

[Andreas Otte]

It is somewhat more complex because there are certain malformed file-urls
possible, like having only two slashes but still having no host like
file://path/file. I think we handle this (or have handled this) by moving the
host into the path-component if parsed as host. Sometimes an UNC path is given
as file://host/path/file when it should have been file://///host/path/file. How
to know what is meant? There is not 100% solution I know of. I think the host
can be savely ignored comparing file urls ... unless it is a malformed UNC path
file-url mentioned above for example. We just can't be sure, so I have to
recommend against ignoring it, just to be on the save side.

Comment 22

[Boris Zbarsky [:bzbarsky]]

So wait.  Are there cases when the Host() of the nsStandardURL would be
different and:

1)  The two URIs should be treated as different, and both resolve to the same
    file
or
2)  The two URIs should be treated as the same and one of the two doesn't resolve
    to a file (or both don't).

In other words, in that diff, should Host() be with the filename/extension/path,
or with scheme/ref/query ?

Comment 23

[Andreas Otte]

Isn't that fun? At the level of the nsStandardUrl we simply don't know in all
cases. That said, if the url is not malformed, everything is fine, the host can
be ignored. But in certain malformed cases, part of the path ends up as host.
This is only resolved (if even possible), if memory serves correctly, while
converting to a local file. 
Usually the host is empty but if not I would put it for file urls with
filename/extension/path. I think this needs some testing with all those strange
border/malformed cases. 

Comment 24

[Boris Zbarsky [:bzbarsky]]

> But in certain malformed cases, part of the path ends up as host.

Ah, I see.  Yeah, in that case we just want to move the Host() check to the
other condition.

Comment 25

[Darin Fisher]

In(In reply to comment #21)
> It is somewhat more complex because there are certain malformed file-urls
> possible, like having only two slashes but still having no host like
> file://path/file. I think we handle this (or have handled this) by moving the
> host into the path-component if parsed as host.

No, we preserve the host in this case.  Try it for yourself ;-)

Loading file://foo/home/darin in the browser shows me my home directory!

The one special case is "file://c:/Windows", and during URL normalization, this
will get converted to "file:///c:/Windows".  By the time, EnsureFile is called,
the URL spec should already be normalized.


> Sometimes an UNC path is given
> as file://host/path/file when it should have been file://///host/path/file. 
> How
> to know what is meant? There is not 100% solution I know of. I think the host
> can be savely ignored comparing file urls ... unless it is a malformed UNC path
> file-url mentioned above for example. We just can't be sure, so I have to
> recommend against ignoring it, just to be on the save side.

This is all hypothetical.  Our implementation does very few fancy things with
malformed file URLs.  We could do a better job of guessing that the user really
meant a UNC path, but we do not.

Comment 26

[Darin Fisher]

Comment on attachment 181324 [details] [diff] [review]
Compiling and all

>Index: netwerk/base/src/nsStandardURL.cpp

>+    // At this point, the URIs are not identical, but they only differ in the
>+    // directory/filename/extension.  If these are file URLs, then get the
>+    // corresponding file objects and compare those, since two filenames that
>+    // differ, eg, only in case could still be equal.
>     if (mSupportsFileURL) {
>         // Assume not equal for failure cases... but failures in GetFile are
>+        // really failures, more or less, so propagate them to caller.
>         *result = PR_FALSE;
>         
>         rv = EnsureFile();
>         if (NS_FAILED(rv)) {
>             LOG(("nsStandardURL::Equals [this=%p spec=%s] failed to ensure file",
>                 this, mSpec.get()));
>             return rv;
>         }
>         NS_ASSERTION(mFile, "EnsureFile() lied!");
>         rv = other->EnsureFile();
>         if (NS_FAILED(rv)) {
>             LOG(("nsStandardURL::Equals [other=%p spec=%s] other failed to ensure file",
>                 other, other->mSpec.get()));
>             return rv;
>         }
>         NS_ASSERTION(other->mFile, "EnsureFile() lied!");
>         return mFile->Equals(other->mFile, result);
>     }

So, this block could be #if defined(XP_WIN) || defined(XP_OS2), right?
Afterall, for unix filesystems this is not required.


>+    // Not file URIs and not identical, so these are different.

I'm having trouble parsing this sentence.  What? ;-)

Comment 27

[Boris Zbarsky [:bzbarsky]]

> So, this block could be #if defined(XP_WIN) || defined(XP_OS2), right?

Maybe... I'd really rather avoid building assumptions like that into this code
if I can avoid it, but it's your call.

> I'm having trouble parsing this sentence.  What? ;-)

The two URIs are not identical, and they're not file URIs.  So Equals() should
test false.  For file URIs, it can test true even if they're not identical
because of the case-folding stuff.  I suppose I could write this out in more
detail... ;)

Comment 28

[Darin Fisher]

Comment on attachment 181324 [details] [diff] [review]
Compiling and all

>+    // Not file URIs and not identical, so these are different.

I get it now.  How about this instead:

  // The URLs are not identical, and they do not correspond to the
  // same file, so they are different.


sr=darin (the #ifdef'ing of that EnsureFile block is optional, and
maybe you have a compelling reason for why we want it on UNIX and
other systems -- e.g., because of error checking?)

Comment 29

[Boris Zbarsky [:bzbarsky]]

Mostly I want it on in general because it makes us easier to port... It's not
obvious to me what should be happening even for OSX, much less other things we
may want to run on sometime so I'd really rather leave it to the system nsIFile
impl unconditionally.

Comment 30

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 181324 [details] [diff] [review]
Compiling and all

Requesting approval for this fix... Should make CSSLoader, at least, happier.

Comment 31

[timeless]

note that you can run linux w/ fat(16/32), osx of course is generally only case
preserving (not case sensitive), plus you could have a fun remote file system
which plays by those rules (and yes, i really do all these things, including
case sensitive ntfs, case sensitive HFSX on osx, and random file sharing among
arbitrary os's including hosting mozilla on the wrong volume or os)

Comment 32

[Brendan Eich [:brendan]]

Comment on attachment 181324 [details] [diff] [review]
Compiling and all

a=brendan for 1.8b2.

/be

Comment 33

[Darin Fisher]

(In reply to comment #31)
> note that you can run linux w/ fat(16/32), osx of course is generally only case
> preserving (not case sensitive), plus you could have a fun remote file system
> which plays by those rules (and yes, i really do all these things, including
> case sensitive ntfs, case sensitive HFSX on osx, and random file sharing among
> arbitrary os's including hosting mozilla on the wrong volume or os)

timeless: if you wish to write code to do a 'proper' file comparison for
nsLocalFileUnix, then by all means.

Comment 34

[Boris Zbarsky [:bzbarsky]]

Fixed (with the comment change)