Closed Bug 888645 Opened 11 years ago Closed 11 years ago

Access navigator.userAgent from jar:file:///path/to/file.zip!/index.html throw NS_ERROR_FAILURE

Categories

(Core :: Networking: HTTP, defect)

Product:
Core
Component:
Networking: HTTP
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla25

People

(Reporter: zbinlin, Assigned: xidorn)

Details

Attachments

(1 file, 2 obsolete files)

patch v3
914 bytes, patch
Gavin
: review+
Details | Diff | Splinter Review 
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:25.0) Gecko/20100101 Firefox/25.0 (Beta/Release)
Build ID: 20130628100353

Steps to reproduce:

1. create index.html:
  <!DOCTYPE html>
  <html>
      <head>
          <meta charset="utf-8">
          <title></title>
      </head>
      <body>
          <script>console.log(navigator.userAgent)</script>
      </body>
  </html>
2. add index.html to file.zip
3. open jar:file:///path/to/file.zip!/index.html.


Actual results:

Throw a error in Error Console:

Error: NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIURI.asciiHost]
Source File: resource://gre/modules/UserAgentOverrides.jsm
Line: 50
OS: Linux → All
To fix it, can catch the exception.
Attached patch patch (obsolete) — — Splinter Review 

        Attachment #769482 -
        Flags: review?(bent.mozilla)

    
    

    
  
Comment on attachment 769482 [details] [diff] [review]
patch

Thanks for the patch!

Changing nsJARURI probably isn't the right approach, it's inconsistent with the nsIURI interface definition and other attributes, and probably has other implications.

UserAgentOverrides should just handle this case by catching the exception.

        Attachment #769482 -
        Flags: review?(bent.mozilla) → review-
Component: Untriaged → General
Hardware: x86_64 → All
Status: UNCONFIRMED → NEW
Ever confirmed: true

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch v2 (obsolete)
        — 
          — Splinter Review
      

    


  
This patch uses the host information of JAR file. I believe it should be better than simply catching the exception in UserAgentOverrides.

        Attachment #769482 -
        Attachment is obsolete: true

        Attachment #769604 -
        Flags: review?(gavin.sharp)

    
    

    
  
Comment on attachment 769604 [details] [diff] [review]
patch v2

No, I think this is still too broad of a change. That some nsIURI attribute getters can throw is a well-known aspect of the API, and users need to handle it. 

Changing the nsJARURI implementation to return aspects of the underlying URI shouldn't be done just to cover up UserAgentOverrides's failure to handle this specific case. It has the potential to cause compatibility issues, and it wouldn't even be a complete "fix" - there are other URI implementations (e.g. nsSimpleURI used for data: URIs) that also have this problem.

        Attachment #769604 -
        Flags: review?(gavin.sharp) → review-

    
    

    
  
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #5)
> Comment on attachment 769604 [details] [diff] [review]
> patch v2
> 
> No, I think this is still too broad of a change. That some nsIURI attribute
> getters can throw is a well-known aspect of the API, and users need to
> handle it. 

I grepped the code, and found nearly everywhere .asciiHost is not wrapped in a try block, which means, though the getters can throw, most time people just assert they will not do so.

> Changing the nsJARURI implementation to return aspects of the underlying URI
> shouldn't be done just to cover up UserAgentOverrides's failure to handle
> this specific case. It has the potential to cause compatibility issues, and
> it wouldn't even be a complete "fix" - there are other URI implementations
> (e.g. nsSimpleURI used for data: URIs) that also have this problem.

No, both nsSimpleURI and nsNullPrincipalURI return an empty string for asciiHost instead of throwing.

I agree that it may not be a complete fix of this bug, but for jar protocol, it has been enough. It might be better to create a new bug and make this bug depend on it?
Version: 17 Branch → Trunk

    
    

    
  
(In reply to Xidorn Quan from comment #6)
> I grepped the code, and found nearly everywhere .asciiHost is not wrapped in
> a try block, which means, though the getters can throw, most time people
> just assert they will not do so.

Granted, it's not a great "feature" of the API and is a bit of a foot gun. But likely not all of those places accept arbitrary nsIURIs, and I'm sure some of them have equivalent alternate checks (for scheme, or instanceof nsIStandardURL, etc.).

> No, both nsSimpleURI and nsNullPrincipalURI return an empty string for
> asciiHost instead of throwing.

Huh, interesting. Certainly a strange quirk (.host throwing when asciiHost doesn't), but I guess asciiHost was introduced more recently and didn't have any compatibility constraints.

> I agree that it may not be a complete fix of this bug, but for jar protocol,
> it has been enough. It might be better to create a new bug and make this bug
> depend on it?

If you want to push for this change separately, I don't have any objection, but given the potential compatibility impact I'm still skeptical that it's worth it (the practical negative effect is "somewhat confusing API", which generally doesn't win when it goes up against compatibility).

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch v3
        — 
          — Splinter Review
      

    


  
Assignee: nobody → quanxunzhen

        Attachment #769604 -
        Attachment is obsolete: true
Status: NEW → ASSIGNED

        Attachment #769992 -
        Flags: review?(gavin.sharp)

    
  

        Attachment #769992 -
        Flags: review?(dao)

    
    

    
  
Comment on attachment 769992 [details] [diff] [review]
patch v3

How does nsIStandardURL ensure that asciiHost can't throw?

    
  
Component: General → Networking: HTTP
Product: Firefox → Core

    
    

    
  
(In reply to Dão Gottwald [:dao] from comment #9)
> Comment on attachment 769992 [details] [diff] [review]
> patch v3
> 
> How does nsIStandardURL ensure that asciiHost can't throw?

Code ensures: http://dxr.mozilla.org/mozilla-central/source/netwerk/base/src/nsStandardURL.cpp#l1073

    
    

    
  
Right - we have only one implementation of nsStandardURL and make assumptions about it. Maybe not ideal, but we do this in other places (assume .host can't throw for nsIStandardURLs), so I think it's fine.

        Attachment #769992 -
        Flags: review?(gavin.sharp) → review+

    
  

        Attachment #769992 -
        Flags: review?(dao)

    
  
Keywords: checkin-needed

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/5f652fba751d
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: