346584 - js file referenced with http:// (stored on remote server) is cached in fastload and not refreshed

Status: NEW

(Core :: XUL, defect)

Description

[Adam Wendt]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.0.4) Gecko/20060608 Ubuntu/dapper-security Firefox/1.5.0.4
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.0.4) Gecko/20060608 Ubuntu/dapper-security Firefox/1.5.0.4

In an .xul overlay file if you write <script src="http://... the file will be downloaded the first time you restart firefox after installing the extension, but the file will be stored in the fastload cache and doesn't check the server for newer version when firefox starts like one would hope.

Reproducible: Always

Comment 1

[Boris Zbarsky [:bzbarsky]]

Don't we have some provisions for checking whether a file has changed before fastloading it?  Why are those not kicking in here?

Comment 2

[Brendan Eich [:brendan]]

We use nsILocalFile to GetLastModifiedTime.  None of this was designed to work with HTTP or other remote-file-access protocols.  Why should it?  Or better: why is it necessary to use a remote file from XUL here?  Does that really work in every other way than this FastLoad issue?

/be

Comment 3

[Boris Zbarsky [:bzbarsky]]

Maybe we should just consider not fastloading things that don't QI to nsILocalFile?

Comment 4

[Mike Shaver (:shaver emeritus)]

Use of remote JS from XUL is increasingly common as people try to get their JSON web-service API peanut butter into their Firefox chocolate, so we should probably do something here to protect against this perma-cache behaviour.  (There are other ways to load those scripts, to be sure, but explicit <script/> is a common enough pattern in HTML-UI cases that we should keep it from breaking in a non-obvious way when people move to XUL.)

bz's suggestion seems like it would be branch-worthy and straightforward, so I'm optimistically ?ing.

Comment 5

[Adam Wendt]

I want to confirm that using remote urls DOES work except for this fastload issue, remote .js and remote .xul seem to work rather well (except for not checking for newer version of the .js)

Comment 6

[Brendan Eich [:brendan]]

The problem is not a missing null test after QI to nsIFile -- the problem is that the only calls to nsIFastLoadService::AddDependency are from the chrome protocol handler implementations (old under rdf/chrome/... and new under chrome).  In the original FastLoad design, the caller of the FastLoad service was required to add dependencies, because only the caller knew when it saw an edge in the dependency graph (which was when slow-loading file A was parsed to the point where it asked for file B).

See http://lxr.mozilla.org/mozilla/source/chrome/src/nsChromeProtocolHandler.cpp#597 for details.  To fix this generically may require more than a small 1.8.1-safe fix.  Possibly nsXULContentSink.cpp could be hacked to do something for non-chrome URIs in nsXULContentSink::OpenScript.

/be

Comment 7

[Mike Schroepfer]

Clearing the flag since this was also in FF1.5 - although if a branch-safe 1.8 patch could be had we'd definitely be game...

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Would comment 3's approach be possible?  i.e., assuming that we need to do a network load for something that doesn't QI to nsILocalFile?

Comment 9

[Brendan Eich [:brendan]]

(In reply to comment #8)
> Would comment 3's approach be possible?  i.e., assuming that we need to do a
> network load for something that doesn't QI to nsILocalFile?

Again, the problem is that nothing QIs to nsILocalFile.  Rather, AddDependency (lxr for it) is called only by the chrome: implementation.  So any inclusion of a script from a different "protocol handler" will not be tracked by a FastLoad file dependency.  IOW, FastLoad dependency accumulation is indirect, observed at a low level, in order to catch all chrome->chrome dependencies at the points where native paths can be (already have been) computed.

One way to proceed is to keep track of muxed document starts, crossing off those URIs that are added as (native pathname) dependencies.  Whatever's left must be treated as an unsafe muxed document, and removed from the FastLoad file.  This is as ugly as it sounds (associating URIs with native pathnames, vetoing the decision to save FastLoad-serialized data after data has been saved).

The direct way that fixes *only* this bug's symptom (<script src="http://...">) is to hack some ad-hoc test into nsXULContentSink.cpp, or perhaps nsScriptLoader.cpp (but that's pretty generic, and now on the trunk, script language agnostic as well as FastLoad-unaware).

FastLoad is really a XUL thing, so nsXULContentSink.cpp still seems best to me.  I will take a stab at this, with a long knife.

/be

Comment 10

[Brendan Eich [:brendan]]

Attachment: stab in the dark

This compiles, but it needs testing love.  I don't have time in the short run to carry it over the finish line (including making a branch version of the final patch). Anyone?

/be