Closed Bug 286702 Opened 20 years ago Closed 19 years ago

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

Categories

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

x86
Windows XP
defect

Tracking

()

RESOLVED FIXED
mozilla1.8beta2

People

(Reporter: timeless, Assigned: bzbarsky)

References

(Blocks 1 open bug)

Details

(Keywords: assertion)

Attachments

(2 files, 3 obsolete files)

###!!! 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.
Blocks: 286705
Please let me know if this is reproducible and what the steps to reproduce are....
Assignee: bzbarsky → nobody
Attachment #178304 - Attachment description: www.macromedia[1].com → www.macromedia[1].com bug 286704
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.
Attached patch Proposed patch (obsolete) — Splinter Review
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...
Assignee: nobody → bzbarsky
Status: UNCONFIRMED → ASSIGNED
Attachment #178323 - Flags: superreview?(darin)
Attachment #178323 - Flags: review?(darin)
Severity: minor → major
Priority: -- → P1
Summary: ###!!! ASSERTION: Bad loading table: 'mLoadingDatas.Get(aLoadData->mURI, &loadingData) && loadingData == aLoadData', file r:/mozilla/layout/style/nsCSSLoader.cpp, line 1486 → [FIX]###!!! ASSERTION: Bad loading table: 'mLoadingDatas.Get(aLoadData->mURI, &loadingData) && loadingData == aLoadData', file r:/mozilla/layout/style/nsCSSLoader.cpp, line 1486
Target Milestone: --- → mozilla1.8beta2
Attachment #178323 - Flags: superreview?(darin)
Attachment #178323 - Flags: review?(darin)
Attached patch Same, but compiling (obsolete) — Splinter Review
Attachment #178323 - Attachment is obsolete: true
Attachment #178325 - Flags: superreview?(darin)
Attachment #178325 - Flags: review?(darin)
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.
Attachment #178325 - Flags: superreview?(darin)
Attachment #178325 - Flags: superreview+
Attachment #178325 - Flags: review?(darin)
Attachment #178325 - Flags: review+
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?
Summary: [FIX]###!!! ASSERTION: Bad loading table: 'mLoadingDatas.Get(aLoadData->mURI, &loadingData) && loadingData == aLoadData', file r:/mozilla/layout/style/nsCSSLoader.cpp, line 1486 → [FIXr]###!!! ASSERTION: Bad loading table: 'mLoadingDatas.Get(aLoadData->mURI, &loadingData) && loadingData == aLoadData', file r:/mozilla/layout/style/nsCSSLoader.cpp, line 1486
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.
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.
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.
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.   
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.
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...
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).
Flags: blocking1.8b2?
Attached patch Proposed patch (obsolete) — Splinter Review
I think this patch is money.  It does the right thing for clearly identical
URIs, and does its best for the rest.
Attachment #178325 - Attachment is obsolete: true
Attachment #181314 - Flags: superreview?(darin)
Attachment #181314 - Flags: review?(cbiesinger)
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?)
Attachment #181314 - Flags: review?(cbiesinger) → review+
> 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.
Attachment #181314 - Attachment is obsolete: true
Attachment #181324 - Flags: superreview?(darin)
Attachment #181314 - Flags: superreview?(darin) → superreview-
> 
> + 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.
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...
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.
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 ?
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. 
> 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.
Flags: blocking1.8b2? → blocking1.8b2+
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 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? ;-)
> 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 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?)
Attachment #181324 - Flags: superreview?(darin) → superreview+
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 on attachment 181324 [details] [diff] [review]
Compiling and all

Requesting approval for this fix... Should make CSSLoader, at least, happier.
Attachment #181324 - Flags: approval1.8b2?
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 on attachment 181324 [details] [diff] [review]
Compiling and all

a=brendan for 1.8b2.

/be
Attachment #181324 - Flags: approval1.8b2? → approval1.8b2+
(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.
Fixed (with the comment change)
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: