Closed Bug 206691 Opened 21 years ago Closed 16 years ago

can't view source of cached xul (view-source chrome:// URLs sometimes don't work)

Categories

(Core :: General, defect)

Product:
Core
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla1.9.2a1

People

(Reporter: rbs, Assigned: neil)

References

(Depends on 1 open bug)

Details

(Keywords: fixed1.9.1, Whiteboard: [fixed1.9.1b4])

Attachments

(3 files, 3 obsolete files)

Updated for review comments (-w)
14.94 KB, patch
benjamin
: review+
bzbarsky
: superreview+
Details | Diff | Splinter Review
Patch as pushed
18.21 KB, patch
beltzner
: approval1.9.1+
Details | Diff | Splinter Review
mochichrome test
2.61 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review 
Need a _DEBUG_ build to reproduce. Steps to reproduce (with a debug build):
1) Open JS Console

2) Open Edit -> Preferences -> Mail & Newsgroups -> Message Display
   A CSS error should appear on the JS console (at the time of filing this bug)

3) Click on the XUL link that shows up in the JS Console (at the time of filing
   this bug, it is chrome://messenger/content/pref-viewing_messages.xul)

Expected Result:
  Should Goto Line

Actual Result:
  Another error appears on the JS console, and following the link (Goto line
  which works here) shows that the .body isn't there. This seems to be what
  christian said in bug 104383 comment 54.
view-source for cached chrome XUL has had issues for a while now; I get no
source coming up even in a build prior to Christian's patch.

And if you look in DOM inspector, there is indeed no <body>, and in fact no
<link> in the <head> either.

So this is not likely to be a front-end problem; this is a problem in either the
HTMLParser back end or maybe even in necko-ish code (XUL cache, etc).
Interestingly, |view-source:| of that-XUL-URL works fine.
view-source:chrome://messenger/content/pref-viewing_messages.xul
It does in my opt build, but not in my debug build...
I put a break in the constructor of nsViewSourceHTML and saw that the XUL is fed
through a |nsCachedChromeChannel|. This channel is hosted here:
http://lxr.mozilla.org/seamonkey/source/rdf/chrome/src/nsChromeProtocolHandler.cpp#89

It doesn't play nice with view-source at all. Fixing this bug requires a way to
tell nsChromeProtocolHandler::NewChannel() that it should use the disk. There is
an existing |else| for that, but it is not direct to get there with view-source.

Alternatively, nsViewSourceChannel::Init(nsIURI* uri) can test if the URI is
chrome, and duplicate what the |else| is doing. But this creates nasty
dependencies between the chrome APIs and necko.

Updating title to reflect the reality. And re-assigning to the defaults.
Assignee: christian → doron
Summary: Viewsource can't goto line in XUL → Can't viewsource of cached xul
OK... so the problem is that chrome just violates the Necko APIs it claims to
implement....

In short, the assumption that the only thing loading XUL documents via necko is
the docshell is completely bogus.  If you have to special-case cached XUL
documents, Necko APIs are not the right place to do it.
Especially since all the code really handling the special-casing lives in
nsXULDocument anyway.... I'd think that using a normal channel for all loads but
cancelling it after the proto document has finished embedding itself would be a
better approach.  Or having chrome channels implement nsIChromeChannel, which
will allow their data-fetching to be shut down (by nsXULDocument) while still
having them fire OnStopRequest...
dup of bug 87918?
Blocks: 78422
*** Bug 87918 has been marked as a duplicate of this bug. ***
Product: Browser → Seamonkey
*** Bug 313109 has been marked as a duplicate of this bug. ***
Blocks: 121006
Assignee: doronr → nobody
Component: ViewSource → XP Toolkit/Widgets: XUL
Product: Mozilla Application Suite → Core
QA Contact: pmac → xptoolkit.xul
Component: XP Toolkit/Widgets: XUL → General
QA Contact: xptoolkit.xul → general
Summary: Can't viewsource of cached xul → Can't view source of cached xul
Summary: Can't view source of cached xul → can't view source of cached xul (view-source chrome:// URLs sometimes don't work)
Attached patch WIP (-w) (obsolete) — — Splinter Review 
I'm going to run with this for a bit, in case I find some regressions. (The first time I tried turning the XUL cache off I got a fastload assertion... I can't reproduce that one reliably though.)

I don't really know whether my nsXULDocument.cpp change is correct.
Attached patch Working patch (-w) (obsolete) — — Splinter Review 
The #ifdef DEBUG code was removed by Shawn for some reason, but I find it useful to detect chrome URL errors.

I don't know whether removing the call to AbortFastLoads is safe.

I also don't know how else to ignore the available data.
Assignee: nobody → neil
Attachment #350665 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #352854 - Flags: superreview?(bzbarsky)
What's the Txul impact?  This actually reads all the XUL data from disk and just doesn't parse copy it out of the necko buffers, doesn't it?  It would be really nice to avoid that if we can by canceling early...
(In reply to comment #13)
> What's the Txul impact?  This actually reads all the XUL data from disk and
> just doesn't parse copy it out of the necko buffers, doesn't it?  It would be
> really nice to avoid that if we can by canceling early...
I tried that, and it works with some windows or not others. If you've got any ideas for how to debug that, I'm all ears.

I'll see if I can work out how to measure the Txul impact, but I only build debug, so I might have to persuade someone else to measure it (try server?).
> I tried that, and it works with some windows or not others. 

You're only canceling in cases when we have cached chrome right?  Past that, I don't know.....

And yes, try server runs Txul (named Twinopen there).
(In reply to comment #15)
> > I tried that, and it works with some windows or not others. 
> You're only canceling in cases when we have cached chrome right?
Well, in the cases where we found a prototype in the XUL cache, yes.

> And yes, try server runs Txul (named Twinopen there).
Which is too noisy for my patch to make a noticable difference.
(In reply to comment #15)
> > I tried that, and it works with some windows or not others. 
> You're only canceling in cases when we have cached chrome right?  Past that, I
> don't know.....
In case it helps, for the two windows where it did not work, they had no overlays (neither static nor dynamic), and the XUL seemed to be all there but none of the script seemed to have run.
Hmm.  That seems pretty odd to me, especially since there is nothing interesting in the OnStopRequest handler here, right?

I do think the patch as written would be a noticeable hit on network filesystems or slow disks (e.g. mobile), so it sounds like we should figure out why canceling this request in OnStartRequest is a problem.
Attached patch Proposed patch (-w) (obsolete) — — Splinter Review 
Perhaps we should have a constant in nsNetError.h
#define NS_BINDING_CACHED
    NS_ERROR_GENERATE_FAILURE(NS_ERROR_MODULE_NETWORK, 5)
and use that everywhere instead, but that's for a followup bug. Speaking of followup, we could also probably remove the "I" from nsIXULPrototypeCache.
Attachment #352854 - Attachment is obsolete: true
Attachment #358702 - Flags: superreview?(bzbarsky)
Attachment #358702 - Flags: review?(enndeakin)
Attachment #352854 - Flags: superreview?(bzbarsky)
So is the key here that the patch that used BINDING_ABORTED ended up not firing onload and that this broke stuff?   That's the only reason I can think of for the imagelib code "doing the right thing" here...
(In reply to comment #20)
> So is the key here that the patch that used BINDING_ABORTED ended up not firing
> onload and that this broke stuff?   That's the only reason I can think of for
> the imagelib code "doing the right thing" here...
Right, since the document viewer fires onerror instead unless you use that magic value. Also biesi pointed out that I should return it in OnStartRequest.
Comment on attachment 358702 [details] [diff] [review]
Proposed patch (-w)

>+++ mozilla/chrome/src/nsChromeProtocolHandler.cpp	Sun Jan 25 12:41:24 2009

>                     rv = fastLoadServ->AddDependency(file);
>-                    if (NS_FAILED(rv))
>-                        cache->AbortFastLoads();
>-                }

Why do we want to remove that?

>+++ mozilla/content/xul/document/src/nsXULDocument.cpp	Sun Jan 25 12:41:24 2009
>+    return NS_IMAGELIB_ERROR_LOAD_ABORTED; // this doesn't really abort the load

Sure it does.  How about:

  "Abort the necko channel, but use a status code that won't prevent us firing onload."

Then make sure you have followup bug filed to have a necko code that means that, and put that bug's number in this comment?
(In reply to comment #22)
>(From update of attachment 358702 [details] [diff] [review])
>>                     rv = fastLoadServ->AddDependency(file);
>>-                    if (NS_FAILED(rv))
>>-                        cache->AbortFastLoads();
>>-                }
>Why do we want to remove that?
Actually this seems to be the wrong place to track fast load dependencies; nsXULPrototypeCache::WritePrototype looks to be a better place to do it.

>>+++ mozilla/content/xul/document/src/nsXULDocument.cpp	Sun Jan 25 12:41:24 2009
>>+    return NS_IMAGELIB_ERROR_LOAD_ABORTED; // this doesn't really abort the load
>Sure it does.
Yeah, I admit, it was the best I could do and still fit in 80 characters.

>Then make sure you have followup bug filed to have a necko code that means
>that, and put that bug's number in this comment?
As I predicted in comment #19 :-)
> Yeah, I admit, it was the best I could do and still fit in 80 characters.

C++ supports multiline comments, last I checked!

Make sure you get review on the fastload dependency thing from someone who actually understands how fastload fits together, ok?  Failing that, I'd prefer we left it in and did a separate bug on removing it; it doesn't seem related to this bug.

        Attachment #358702 -
        Attachment is obsolete: true

        Attachment #358846 -
        Flags: superreview?(bzbarsky)

        Attachment #358846 -
        Flags: review?(enndeakin)

        Attachment #358702 -
        Flags: superreview?(bzbarsky)

        Attachment #358702 -
        Flags: review?(enndeakin)

        Attachment #358846 -
        Flags: superreview?(bzbarsky) → superreview+

    
    

    
  
I really have no idea what this patch or the code does.

    
    

    
  
For what it's worth, I could probably just r+sr this if that's ok with enn and Benjamin.

    
  

        Attachment #358846 -
        Flags: review?(enndeakin) → review+

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch as pushed
        — 
          — Splinter Review
      

    


  

    
    

    
  
Pushed changeset 908b22d8fefe to mozilla-central.
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED

    
    

    
  
Comment on attachment 359645 [details] [diff] [review]
Patch as pushed

Since I landed this I've discovered that it fixes a number other bugs not necessarily listed in Bugzilla, for instance manually visiting chrome://global/content/config.xul stops about:config from working. It also removes the race between nsChromeProtocolHandler and nsXULDocument that causes bug 321099 amongst others.

        Attachment #359645 -
        Flags: approval1.9.1?

    
    

    
  
Comment on attachment 359645 [details] [diff] [review]
Patch as pushed

Firefox 3.0 might want this for the increased security and stability this offers.

        Attachment #359645 -
        Flags: approval1.9.0.8?

    
    

    
  
Comment on attachment 359645 [details] [diff] [review]
Patch as pushed

We'd want a change like this to land on 1.9.1 before we'd take it on the stable branch. We'll check back later?

        Attachment #359645 -
        Flags: approval1.9.0.8?
Flags: wanted1.9.0.x+

    
    

    
  
Comment on attachment 359645 [details] [diff] [review]
Patch as pushed

a191=beltzner

This needs to either be added to Litmus or have a test come along with it.

        Attachment #359645 -
        Flags: approval1.9.1? → approval1.9.1+

    
    

    
  

      
      
      
      

        Attached patch
          
          
        mochichrome test
        — 
          — Splinter Review
      

    


  

        Attachment #368344 -
        Flags: review?(bzbarsky)

        Attachment #368344 -
        Flags: review?(bzbarsky) → review+

    
    

    
  
Comment on attachment 368344 [details] [diff] [review]
mochichrome test

Pushed changeset 834d31edda46 to mozilla-central.

    
    

    
  
Pushed changeset f0deff7035c6 to releases-mozilla1.9.1 (last two attachments).
Keywords: fixed1.9.1
Depends on: 484391
Whiteboard: [fixed1.9.1b4]
Target Milestone: --- → mozilla1.9.2a1

    
  

        Attachment #359645 -
        Flags: approval1.9.0.12?
Blocks: 321099
Blocks: 388887

    
    

    
  
Comment on attachment 359645 [details] [diff] [review]
Patch as pushed

I don't think we care about this for Firefox 3.0.x -- users that hit problems (most likely developer types anyway) can upgrade to 3.5

        Attachment #359645 -
        Flags: approval1.9.0.12?

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: