Closed Bug 376222 Opened 16 years ago Closed 15 years ago

A javascript error in an extension's progress listener shouldn't interfere with normal operation of tabbed browsing

Categories

(Firefox :: Tabbed Browser, defect, P4)

defect

Tracking

()

RESOLVED FIXED

People

(Reporter: john.p.baker, Assigned: john.p.baker)

References

Details

Attachments

(3 files, 1 obsolete file)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a4pre) Gecko/20070327 Minefield/3.0a4pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a4pre) Gecko/20070327 Minefield/3.0a4pre

The progress listener handlers are independent of each other and should be isolated so that
a) they don't interfere with normal tabbed browsing operation
b) they don't interfere with each other

Reproducible: Always

Steps to Reproduce:
1. Install (pathological) extension (to follow)
2. Browse normally
3.
Actual Results:  
Some unusual behaviour - for instance
o the window title doesn't update.
o the tabbar doesn't position properly when overflowing

Expected Results:  
No noticable change.

Normally these effects will be fairly random but I do I have one, real life, extension that can reproducibly exhibit this problem but I don't particularly want to go into specifics for this bug.

There is a tendency to dismiss all bugs if they disappear when replicated in a clean profile however, in this case, I feel that the tabbrowser has some responsibility in maintaining normal operation. I would propose that the dispatchers should be changed to something like this:

  if (p)
+   try {
      p.onFooChange(....);
+   } catch (e) {
+     // don't inhibit other listeners
+     // XXX log exception to console?
+   }
There's a tendency to dismiss bugs reported by extension *users* who have no idea where the problem lies - for extension *authors* we just wait expectantly for them to decide they'll have to provide not just the diagnosis, but also the patch ;)
Attached patch suggested patch (obsolete) — Splinter Review
Sorry :) I was probably a little too keen to show that this shouldn't be treated as an extension bug - I have contacted Myk separately - but a bug in how tabbrowser treats errors in extensions.

Here is my suggested patch:
o Empty catches seem to be done in three different ways in this file
o It wasn't obvious that "onUpdateCurrentBrowser", "onRefreshAttempted" and "onLinkIconAvailable" should be included, but the first two do have following actions.
o Only the "... or following code" paths are tested by the included extension
Attached patch diff -wSplinter Review
... because of the indentation changes.
Gavin, Mano - what do you think? Is he hooked enough now that we can bring up how he needs to write automated testcases for most of tabbrowser, too?
Probably better to attack ProgressListener documentation then - suggesting that extensions protect the listeners instead.

However it would be nice to find a way of going through the missed code in updateCurrentBrowser, as that is the nasty one breaking:
o fastfind
o title bar
o TabSelect
o throbber
(In reply to comment #5)
> he needs to write automated testcases for most of tabbrowser, too?

He doesn't need to "write automated testcases for most of tabbrowser". The extension he provided should be easy to turn into a single chrome MochiTest, and Mano or I (or whoever ends up reviewing the patch) can help with that if needed.
I see this bug in my Content Preferences extension, for example when it runs into bug 375918.
Comment on attachment 260558 [details] [diff] [review]
suggested patch

A number of folks testing out beta1 have reported that the location bar doesn't get focused when they open a new tab, and I think this bug is likely the culprit, for folks who are testing with extensions.checkCompatibility set to false and whose extensions haven't been updated to work with Firefox 3.  We should try to get this fix in.
Attachment #260558 - Flags: review?(gavin.sharp)
Requesting wanted-firefox3 for this bug that causes busted extensions to make it look like Firefox itself is busted (which could be the cause of bug 400131 and bug 404011).
Flags: blocking-firefox3?
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: wanted-firefox3+
Flags: blocking-firefox3?
Flags: blocking-firefox3-
Priority: -- → P4
Attachment #260559 - Attachment is patch: true
Attachment #260558 - Flags: review?(gavin.sharp) → review+
Here's a version of the patch that has been updated to resolve a trivial conflict with the current trunk.  This is the version that should be checked in.

Requesting approval to land this wanted bug that resolves a variety of problems caused by broken or buggy extensions that implement progress listeners and throw exceptions when notified about progress.

Currently, we don't catch those exceptions, we just stop notifying subsequent progress listeners, which means that one buggy extension can prevent all subsequent progress listeners (including core Firefox listeners that do things like focus the location bar when the user opens a new tab) from running.

With this patch, we'll catch such exceptions and continue notifying subsequent progress listeners, so buggy extension (and core Firefox, for that matter) listeners can't unintentionally hork other Firefox and extension listeners.
Attachment #260558 - Attachment is obsolete: true
Attachment #306638 - Flags: approval1.9?
Comment on attachment 306638 [details] [diff] [review]
patch updated to resolve conflict

a1.9=beltzner
Attachment #306638 - Flags: approval1.9? → approval1.9+
Assignee: nobody → john.p.baker
Keywords: checkin-needed
Version: unspecified → Trunk
Thanks for the fix, John!

Checking in browser/base/content/tabbrowser.xml;
/cvsroot/mozilla/browser/base/content/tabbrowser.xml,v  <--  tabbrowser.xml
new revision: 1.264; previous revision: 1.263
done
Status: NEW → RESOLVED
Closed: 15 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Blocks: 421245
Just thought I'd mention I saw a reference to this Bug at line 4005 of browser.js
--
4005     // Catch exceptions until bug 376222 gets fixed so we don't hork
4006     // other progress listeners if this call throws an exception.
---
http://mxr.mozilla.org/mozilla/source/browser/base/content/browser.js#4005
You need to log in before you can comment on or make changes to this bug.