Closed Bug 415367 Opened 14 years ago Closed 14 years ago

ieTab extension not working due to use of ":" in chrome URIs

Categories

(Core :: General, defect)

defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla1.9beta3

People

(Reporter: ehsan.akhgari, Assigned: ehsan.akhgari)

References

()

Details

(Keywords: regression, Whiteboard: fixed by bug 415338)

Attachments

(1 file, 2 obsolete files)

Spinoff from bug 415218 comment 16.

The ieTab extension does not work on trunk, because it uses URIs in form of <chrome://ietab/content/reloaded.html?url=http://www.google.com/> and the code highlighted in the URL of this bug blindly rejects the ":" character inside chrome URIs.  This seems to be a regression from bug 413250, which affects both trunk and 1.8.1.12.  So, this may be observable when Firefox 2.0.0.12 is released.  I'm currently downloading 2.0.0.11rc4 to test this.

This should be fixed in Firefox 3 beta 3 as well, so I'm setting the target milestone appropriately to get the appropriate attention.

To test the problem, enter the following code in the error console:

var ios = Components.classes["@mozilla.org/network/io-service;1"].getService(Components.interfaces.nsIIOService); alert(ios.newURI("chrome://blah/blah/?url=%3A", null, null).spec);

The error message observed in the console:

Error: uncaught exception: [Exception... "Access to restricted URI denied"  code: "1012" nsresult: "0x805303f4 (NS_ERROR_DOM_BAD_URI)"  location: "javascript:%20var%20ios%20=%20Components.classes["@mozilla.org/network/io-service;1"].getService(Components.interfaces.nsIIOService);%20alert(ios.newURI("chrome://blah/blah/?url=%253A",%20null,%20null).spec); Line: 1"]
Flags: in-testsuite?
Flags: blocking1.9?
Flags: blocking1.8.1.12?
Reversing direction of the dependency (Given that all the patches are in Bug 413250).
No longer blocks: 413250
Depends on: 413250
It seems like branch already handles this correctly:

<http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/chrome/src/nsChromeRegistry.cpp&rev=1.338.2.8&root=/cvsroot&mark=689-692#668>

I'm preparing a patch for trunk with a unit test.
Assignee: nobody → ehsan.akhgari
Attached patch Patch (v1) (obsolete) — Splinter Review
First take.
Attachment #301032 - Flags: superreview?(neil)
Attachment #301032 - Flags: review?(neil)
This gets fixed by Bug 415338.  Although that doesn't have a test.
Attached patch Patch (v2) (obsolete) — Splinter Review
The unit test of the previous patch was mistakenly included in the patch, and was not the unit test that I got to pass.  Here is the updated patch with the correct unit test which passes with the patch applied.
Attachment #301032 - Attachment is obsolete: true
Attachment #301035 - Flags: superreview?(neil)
Attachment #301035 - Flags: review?(neil)
Attachment #301032 - Flags: superreview?(neil)
Attachment #301032 - Flags: review?(neil)
OK, this doesn't affect branch, sorry for the bug spam.  But it still affects trunk and should be fixed for beta 3 because it can break many extensions.
No longer blocks: 414327
Status: NEW → ASSIGNED
Flags: blocking1.8.1.12?
(In reply to comment #1)
> Reversing direction of the dependency (Given that all the patches are in Bug
> 413250).

By convention we make regressions "block" the causing bug, not the other way around. You can argue either way, but sticking with convention makes it less likely regressions will be missed when patches are ported to branches.
Blocks: 413250
Depends on: 413338
No longer depends on: 413250
Whiteboard: fixed by bug 415338
Depends on: 415338
No longer depends on: 413338
Since the patch in bug 415338 got approval I've checked it in.
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Attached patch Unit testSplinter Review
The unit test should still be worth considering I guess.
Attachment #301035 - Attachment is obsolete: true
Attachment #301039 - Flags: superreview?(dveditz)
Attachment #301039 - Flags: review?(dveditz)
Attachment #301035 - Flags: superreview?(neil)
Attachment #301035 - Flags: review?(neil)
Comment on attachment 301039 [details] [diff] [review]
Unit test

r/sr=dveditz
Attachment #301039 - Flags: superreview?(dveditz)
Attachment #301039 - Flags: superreview+
Attachment #301039 - Flags: review?(dveditz)
Attachment #301039 - Flags: review+
Comment on attachment 301039 [details] [diff] [review]
Unit test

This unit test should be used to make sure bugs such as this one don't appear in the future.  Seeking approval to land this test.
Attachment #301039 - Flags: approval1.9?
Comment on attachment 301039 [details] [diff] [review]
Unit test

You don't ever need approval for tests.
Attachment #301039 - Flags: approval1.9?
RCS file: /cvsroot/mozilla/chrome/test/unit/test_bug415367.js,v
done
Checking in chrome/test/unit/test_bug415367.js;
/cvsroot/mozilla/chrome/test/unit/test_bug415367.js,v  <--  test_bug415367.js
initial revision: 1.1
done
Keywords: checkin-needed
Flags: in-testsuite? → in-testsuite+
Flags: blocking1.9?
You need to log in before you can comment on or make changes to this bug.