Closed Bug 1268285 Opened 8 years ago Closed 8 years ago

[e10s] Browser breaks when I open invalid link in a new tab

Categories

(Firefox :: Tabbed Browser, defect, P3)

defect

Tracking

()

VERIFIED FIXED
Firefox 49
Tracking Status
e10s + ---
firefox48 + fixed
firefox49 + verified

People

(Reporter: arni2033, Assigned: Gijs)

References

Details

(Keywords: regression)

Attachments

(1 file)

>>>   My Info:   Win7_64, Nightly 49, 32bit, ID 20160426044609
STR_1:
1. Open new window (for your own safety), make sure there's only one tab about:home in that window
2. Type  "chrome://skin/images/"  without quotes in urlbar, press Alt+Enter
3. Close selected tab

AR:  No tabs in tabs toolbar, only [+] button
ER:  Window should close


This is regression from bug 1243707. Regression range:
> https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=8046e825980ac4080619527b72292b8e9ac1f56d&tochange=5ae810198a9c8c7f4e75cd0881bfc8d1295cbc8d


STR_2 (explains how exactly the browser is broken):
1. Open new window with only 2 tabs - 2 about:home tabs. Select the second tab
2. Type  "chrome://skin/images/"  without quotes in urlbar, press Alt+Enter
3. Press Ctrl+PageDown
4. Press Ctrl+PageDown
5. Click menu (≡) -> Options. Switch to 1st window, click (≡)->Options. Switch back to 2nd window
6. Close two opened tabs
7. Click [+] button in tabs toolbar
8. Wait 1 minute, then close the window opened in Step 1 (if you can). Wait 5 minutes.

AR:
 Step 3 - nothing. Step 4 - 1st tab is selected. Step 5 - nothing. Step 6 - 0 tabs in tabs toolbar.
 Step 7 - firefox.exe consumes 25% CPU. Step 8 - firefox.exe still consumes 25% CPU.

ER:
 Step 3 - Switch to 1st tab. Step 4 - switch to 2nd tab. Step 5 - Options should open.
 Step 6 - 2nd window should close
Do you see any errors in the browser console?
Flags: needinfo?(arni2033)
  [*STR_1 - Step 2*]
----(unknown)
[Exception... "Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsIURIFixup.createFixupURI]"  nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)"  location: "JS frame :: resource:///modules/PlacesUIUtils.jsm :: PUIU_createFixedURI :: line 95"  data: no]
PUIU_createFixedURI()   		PlacesUIUtils.jsm:95
PUIU_markPageAsTyped()  		PlacesUIUtils.jsm:520
addToUrlbarHistory()    		browser.js:3840
continueOperation()     		urlbarBindings.xml:387
handleCommand/<()       		urlbarBindings.xml:377
_canonizeURL/<()    			urlbarBindings.xml:526
Handler.prototype.process()		Promise-backend.js:937
this.PromiseWalker.walkerLoop()	Promise-backend.js:816
bound ()						self-hosted
bound bound ()				    self-hosted


----E10SUtils.jsm:55
A promise chain failed to handle a rejection. Did you forget to '.catch', or did you forget to 'return'?
See https://developer.mozilla.org/Mozilla/JavaScript_code_modules/Promise.jsm/Promise

Date: Thu Apr 28 2016 13:33:01 GMT+0300
Full Message: Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsIIOService2.newURI]
Full Stack: JS frame :: resource://app/modules/E10SUtils.jsm :: this.E10SUtils.canLoadURIInProcess :: line 55
JS frame :: chrome://browser/content/tabbrowser.xml :: _linkBrowserToTab :: line 1852
JS frame :: chrome://browser/content/tabbrowser.xml :: addTab :: line 2033
JS frame :: chrome://browser/content/tabbrowser.xml :: loadOneTab :: line 1489
JS frame :: chrome://browser/content/utilityOverlay.js :: openLinkIn :: line 358
JS frame :: chrome://browser/content/utilityOverlay.js :: openUILinkIn :: line 196
JS frame :: chrome://browser/content/urlbarBindings.xml :: continueOperation :: line 442
JS frame :: chrome://browser/content/urlbarBindings.xml :: handleCommand/< :: line 377
JS frame :: chrome://browser/content/urlbarBindings.xml :: _canonizeURL/< :: line 526
JS frame :: resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js :: Handler.prototype.process :: line 937
JS frame :: resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js :: this.PromiseWalker.walkerLoop :: line 816

  [*STR_1 - Step 3*]
----TabState.jsm:104:1
TypeError: browser is undefined
Flags: needinfo?(arni2033)
Allasso, would you mind looking into this?
Flags: needinfo?(allassopraise)
Surely, but I can't right away as I am away on a family emergency right now.
Flags: needinfo?(allassopraise)
I have tried this on latest Nightly on Mac and Windows, and cannot reproduce.

I suspect many of the steps are superfluous as the process is quite convoluted, and some of them don't make sense (ie, pasting chrome://skin/images/ in urlbar and pressing Alt+Enter (or enter) simply removes the url from the bar even in pre-1243707).

Can you please reduce these steps to the simplest path possible to reproduce the problem?
Flags: needinfo?(arni2033)
I can reproduce on latest Nightly49.0a1
https://hg.mozilla.org/mozilla-central/rev/86730d0a82093d705e44f33a34973d28b269f1ea
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:49.0) Gecko/20100101 Firefox/49.0 ID:20160428030218


STR
1. Make sure that there is a tab1, now, there is [about:home][+]
2. Paste chrome://skin/images/ in LocationBar and Alt+ENTER
   ---- observe: nothing happens, but error[1] is shown in Browser Console
3. Close tab1
   ---- observe: there is [+] only, and error[2] is shown in Browser Console
4. Click [+] to open New Tab
   ---- opverve: now, [about:newtab][+], and error[3] is shown in Browser Console
5. Quit Browser from File > exit
   ---- observe: CPU 100% core usage, and Browser will crash[4] after for a minite



[1]
Key event not available on some keyboard layouts: key="c" modifiers="accel,alt"browser.xul
Key event not available on some keyboard layouts: key="i" modifiers="accel,alt,shift"browser.xul
[Exception... "Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsIURIFixup.createFixupURI]"  nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)"  location: "JS frame :: resource:///modules/PlacesUIUtils.jsm :: PUIU_createFixedURI :: line 95"  data: no]
(unknown)
TypeError: browser is undefinedTabState.jsm:104:1
A promise chain failed to handle a rejection. Did you forget to '.catch', or did you forget to 'return'?
See https://developer.mozilla.org/Mozilla/JavaScript_code_modules/Promise.jsm/Promise

Date: Thu Apr 28 2016 23:59:16 GMT+0900
Full Message: Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsIIOService2.newURI]
Full Stack: JS frame :: resource://app/modules/E10SUtils.jsm :: this.E10SUtils.canLoadURIInProcess :: line 55
JS frame :: chrome://browser/content/tabbrowser.xml :: _linkBrowserToTab :: line 1852
JS frame :: chrome://browser/content/tabbrowser.xml :: addTab :: line 2033
JS frame :: chrome://browser/content/tabbrowser.xml :: loadOneTab :: line 1489
JS frame :: chrome://browser/content/utilityOverlay.js :: openLinkIn :: line 358
JS frame :: chrome://browser/content/utilityOverlay.js :: openUILinkIn :: line 196
JS frame :: chrome://browser/content/urlbarBindings.xml :: continueOperation :: line 442
JS frame :: chrome://browser/content/urlbarBindings.xml :: handleCommand/< :: line 377
JS frame :: chrome://browser/content/urlbarBindings.xml :: _canonizeURL/< :: line 526
JS frame :: resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js :: Handler.prototype.process :: line 937
JS frame :: resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js :: this.PromiseWalker.walkerLoop :: line 816E10SUtils.jsm:55



[2]
TypeError: browser is undefined1TabState.jsm:104:1


[3]
TypeError: browser is undefinedTabState.jsm:104:1
Warning: NetUtil.asyncFetch() requires the channel to have one of the security flags set in the loadinfo (see nsILoadInfo). Please create channel using NetUtil.newChannel()
TypeError: browser is null
tabbrowser.xml:3330:19
TypeError: tab.linkedBrowser is null
tabbrowser.xml:3836:17
TypeError: this.browsers[i] is undefined
tabbrowser.xml:411:1
TypeError: this.browsers[i] is undefined
tabbrowser.xml:411:1
Warning: NetUtil.asyncFetch() requires the channel to have one of the security flags set in the loadinfo (see nsILoadInfo). Please create channel using NetUtil.newChannel()
TelemetryStopwatch: key "FX_SESSION_RESTORE_COLLECT_DATA_MS" was already initialized
TelemetryStopwatch.jsm:282
TelemetryStopwatch: key "FX_SESSION_RESTORE_COLLECT_DATA_LONGEST_OP_MS" was already initialized
TelemetryStopwatch.jsm:282
TelemetryStopwatch: key "FX_SESSION_RESTORE_COLLECT_ALL_WINDOWS_DATA_MS" was already initialized
TelemetryStopwatch.jsm:282
TypeError: browser is undefinedTabState.jsm:104:1


[4]
bp-e88fe48d-95cc-43d2-94cc-3886d2160428
I was able to reproduce on windows. This requires the use of the chrome url afaict, it won't fail with a web address.

1) new window, navigate to about:home
2) chrome://skin/images/ + alt+return
3) close tab using tab x

funky browser with no tabs.

Click the + and you get a tab, click close window and it closes.

polish, non-blocking imo.
(In reply to Jim Mathies [:jimm] from comment #7)
> I was able to reproduce on windows. This requires the use of the chrome url
> afaict, it won't fail with a web address.

I haven't tested this, but I wouldn't be surprised if it failed in the same way with anything that's not a valid URI if you've e.g. got keyword.enabled turned off.
Still unable to reproduce on Mac or Windows.  I get errors but nothing related to _linkBrowserToTab or addTab.  No browser crashing.

[Exception... "Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsIURIFixup.createFixupURI]"  nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)"  location: "JS frame :: resource:///modules/PlacesUIUtils.jsm :: PUIU_createFixedURI :: line 95"  data: no]
PUIU_createFixedURI()
PlacesUIUtils.jsm:95
PUIU_markPageAsTyped()
PlacesUIUtils.jsm:520
addToUrlbarHistory()
browser.js:3840
continueOperation()
urlbarBindings.xml:387
handleCommand/<()
urlbarBindings.xml:377
_canonizeURL/<()
urlbarBindings.xml:526
Handler.prototype.process()
Promise-backend.js:937
this.PromiseWalker.walkerLoop()
Promise-backend.js:816
bound ()
self-hosted
bound bound ()
self-hosted
(unknown)
A promise chain failed to handle a rejection. Did you forget to '.catch', or did you forget to 'return'?
See https://developer.mozilla.org/Mozilla/JavaScript_code_modules/Promise.jsm/Promise

Date: Thu Apr 28 2016 09:55:21 GMT-0600 (Mountain Standard Time)
Full Message: Error: page-thumbnail:error
Full Stack: observe@resource://gre/modules/BackgroundPageThumbs.jsm:112:22
BackgroundPageThumbs._onCaptureOrTimeout@resource://gre/modules/BackgroundPageThumbs.jsm:275:7
Capture.prototype._done/done@resource://gre/modules/BackgroundPageThumbs.jsm:413:7
Capture.prototype._done@resource://gre/modules/BackgroundPageThumbs.jsm:425:7
Capture.prototype.notify@resource://gre/modules/BackgroundPageThumbs.jsm:390:5
BackgroundPageThumbs.jsm:112
Warning: NetUtil.asyncFetch() requires the channel to have one of the security flags set in the loadinfo (see nsILoadInfo). Please create channel using NetUtil.newChannel()
A promise chain failed to handle a rejection. Did you forget to '.catch', or did you forget to 'return'?
See https://developer.mozilla.org/Mozilla/JavaScript_code_modules/Promise.jsm/Promise

Date: Thu Apr 28 2016 09:55:51 GMT-0600 (Mountain Standard Time)
Full Message: Error: page-thumbnail:error
Full Stack: observe@resource://gre/modules/BackgroundPageThumbs.jsm:112:22
BackgroundPageThumbs._onCaptureOrTimeout@resource://gre/modules/BackgroundPageThumbs.jsm:275:7
Capture.prototype._done/done@resource://gre/modules/BackgroundPageThumbs.jsm:413:7
Capture.prototype._done@resource://gre/modules/BackgroundPageThumbs.jsm:425:7
Capture.prototype.notify@resource://gre/modules/BackgroundPageThumbs.jsm:390:5
BackgroundPageThumbs.jsm:112
tracking-e10s: --- → +
Priority: -- → P3
The issue here turns out to be very simple: before bug 1243707, opening a new tab created the "remote" boolean before the tab got appended to the tabbrowser.

Trying to run that code:

            let remote = gMultiProcessBrowser &&
                         !aForceNotRemote &&
                         E10SUtils.canLoadURIInProcess(aURI, Ci.nsIXULRuntime.PROCESS_TYPE_CONTENT);

actually throws because:

                         E10SUtils.canLoadURIInProcess(aURI, Ci.nsIXULRuntime.PROCESS_TYPE_CONTENT);

throws because trying to create a URI for "chrome://skin/images/" throws:

[Exception... "Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsIIOService2.newURI]"  nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)"  location: "JS frame :: debugger eval code :: <TOP_LEVEL> :: line 1"  data: no]

That code used to run before the new tab element got stuck on the tabbrowser. Now it runs after that happens, but before we've managed to hook up a <browser>, and so we end up with a tab with no browser connected to it, and tabbrowser gets all kinds of unhappy.

The trivial fix would be to move this back into addTab and pass |remote| on to _linkBrowserToTab as a given, rather than something it has to determine. Dão, does that sound sensible ?
Flags: needinfo?(dao+bmo)
Is there a good reason for E10SUtils.canLoadURIInProcess to throw rather than swallowing the exception and returning true?
Flags: needinfo?(dao+bmo)
(In reply to Dão Gottwald [:dao] from comment #12)
> Is there a good reason for E10SUtils.canLoadURIInProcess to throw rather
> than swallowing the exception and returning true?

Not sure. Felipe ? Does it matter what process we load error pages in and/or can we ignore errors from canLoadURIInProcess?
Flags: needinfo?(felipc)
Regression from 48, tracking.
So, IIUIC, this throws only because the URL used here is invalid, right? And not because it's chrome://. In other words, a valid chrome:// url wouldn't throw, correct?

I don't think it matters which process the error pages loads, as long as the page works correctly on both processes (does it have any interactive buttons or links?)
Flags: needinfo?(felipc)
(In reply to :Felipe Gomes (needinfo me!) from comment #15)
> So, IIUIC, this throws only because the URL used here is invalid, right? And
> not because it's chrome://. In other words, a valid chrome:// url wouldn't
> throw, correct?

Yes.

> I don't think it matters which process the error pages loads, as long as the
> page works correctly on both processes (does it have any interactive buttons
> or links?)

Nothing ends up getting loaded for bad chrome URIs. You end up with a tab with an empty title and about:blank loaded (when turning off e10s). Other "invalid URIs" do have buttons etc., but it looks like we only call newURI for chrome URIs. :-\
Comment on attachment 8751226 [details]
MozReview Request: Bug 1268285 - E10SUtils shouldn't throw when passed invalid chrome: URIs, r?felipe

https://reviewboard.mozilla.org/r/51915/#review48789
Attachment #8751226 - Flags: review?(felipc) → review+
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/45daaf6edeae
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Affected:   Win7_64, Nightly 49, 32bit, ID 20160511030221 (2016-05-11)
Fixed on:   Win7_64, Nightly 49, 32bit, ID 20160512030253 (2016-05-12)
            Win7_64, Nightly 49, 32bit, ID 20160526082509 (2016-05-26)

No crashes and other strangeness with browser.
Status: RESOLVED → VERIFIED
Gijs, are you going to request uplift this to 48? Thanks
Flags: needinfo?(gijskruitbosch+bugs)
Comment on attachment 8751226 [details]
MozReview Request: Bug 1268285 - E10SUtils shouldn't throw when passed invalid chrome: URIs, r?felipe

Approval Request Comment
[Feature/regressing bug #]: bug 1243707
[User impact if declined]: tabbrowser gets confused if you open a tab with an invalid chrome: URI.
[Describe test coverage new/current, TreeHerder]: the tabbrowser as a whole has coverage, this specific part edgecase does not.
[Risks and why]: low risk, avoids throwing in certain situations from a utility component
[String/UUID change made/needed]: no.
Flags: needinfo?(gijskruitbosch+bugs)
Attachment #8751226 - Flags: approval-mozilla-aurora?
Comment on attachment 8751226 [details]
MozReview Request: Bug 1268285 - E10SUtils shouldn't throw when passed invalid chrome: URIs, r?felipe

Fix a leak, taking it
Attachment #8751226 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.