Closed Bug 957922 Opened 10 years ago Closed 10 years ago

"aBrowser is null" in _setPluginNotificationIcon

Categories

(Firefox :: Address Bar, defect)

x86_64
macOS
defect
Not set
trivial

Tracking

()

RESOLVED FIXED
Firefox 29

People

(Reporter: jruderman, Assigned: bhagya.ravikumar)

Details

(Keywords: testcase, Whiteboard: [good first bug][lang=JS][mentor=gfritzsche])

Attachments

(2 files, 2 obsolete files)

Attached file testcase
JavaScript error: chrome://browser/content/browser.js, line 4192: aBrowser is null
Error is here: http://hg.mozilla.org/mozilla-central/annotate/9409405e0739/browser/base/content/browser-plugins.js#l922

Probably called from one of these locations:
http://hg.mozilla.org/mozilla-central/annotate/9409405e0739/browser/base/content/browser-plugins.js#l274
http://hg.mozilla.org/mozilla-central/annotate/9409405e0739/browser/base/content/browser-plugins.js#l377

Because this is in an event handler, this isn't going to break other code, but we should probably null-check and early-return if getBrowserForDocument returns null.
Severity: normal → trivial
gfritzsche, you interested in mentoring somebody for this?
Component: Plugin Finder Service → Location Bar
Product: Toolkit → Firefox
Whiteboard: [good first bug][lang=JS]
Sure, why not.
Whiteboard: [good first bug][lang=JS] → [good first bug][lang=JS][mentor=gfritzsche]
I would like to work on this bug. Can I be assigned to it?
Sure!

Once you have successfully built your own Firefox [1], i would start by making sure you can reproduce the error with the test-case; you'll see it in the "browser console" (or the terminal if you started Firefox from there).
Then you can try out the change from comment 1 - you only need to "mach build browser/base/content" after changing that file.

If anything is unclear, feel free to contact me directly or ask for help on IRC in #introduction.

[1]: https://developer.mozilla.org/en-US/docs/Simple_Firefox_build
Assignee: nobody → bhagya.ravikumar
I completed the build and found the file "browser/base/content/browser-plugins.js" and "browser/base/content/browser.js". What should I do next?
Bhagaya, i took a look at the other error ("aTab is null") and filed a new bug for that (bug 960492), so i think the patch you had is good to go.
Attachment #8361038 - Flags: review?(georg.fritzsche)
Comment on attachment 8361038 [details] [diff] [review]
Bug 957922 - "aBrowser is null" in _setPluginNotificationIcon

Review of attachment 8361038 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good to me, but i'm not a peer. Let's see if jaws can review this.
Attachment #8361038 - Flags: review?(jaws)
Attachment #8361038 - Flags: review?(georg.fritzsche)
Attachment #8361038 - Flags: feedback+
Comment on attachment 8361038 [details] [diff] [review]
Bug 957922 - "aBrowser is null" in _setPluginNotificationIcon

Review of attachment 8361038 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the following changes. Looks good!

::: browser/base/content/browser-plugins.js
@@ +270,5 @@
>  
>      if (eventType == "PluginRemoved") {
>        let doc = event.target;
>        let browser = gBrowser.getBrowserForDocument(doc.defaultView.top.document);
> +      if (browser != null)

if (browser)

@@ +301,5 @@
>      }
>  
>      let shouldShowNotification = false;
>      let browser = gBrowser.getBrowserForDocument(doc.defaultView.top.document);
> +    if (browser == null)

if (!browser)
Attachment #8361038 - Flags: review?(jaws) → review+
Status: NEW → ASSIGNED
Bhagya, can you do the two minor changes requested in comment 10 and put a new patch up?
Attached patch bug-957922 (obsolete) — Splinter Review
Attachment #8361038 - Attachment is obsolete: true
Attachment #8365903 - Flags: review?(georg.fritzsche)
Comment on attachment 8365903 [details] [diff] [review]
bug-957922

This looks good. You already got review from jaws if you do the minor changes, so the message should have "r=jaws" not "r=gfritzsche".

Change that, put the changed patch up and add the "checkin-needed" keyword [1] and it will be good to go! :)

[1] https://developer.mozilla.org/en-US/docs/Developer_Guide/How_to_Submit_a_Patch#Getting_the_patch_checked_into_the_tree
Attachment #8365903 - Flags: review?(georg.fritzsche) → feedback+
Attachment #8365903 - Attachment is obsolete: true
Attachment #8365924 - Flags: checkin?(jaws)
Attachment #8365924 - Flags: checkin?(jaws)
https://hg.mozilla.org/integration/fx-team/rev/046b5f9f5822
Keywords: checkin-needed
Whiteboard: [good first bug][lang=JS][mentor=gfritzsche] → [good first bug][lang=JS][mentor=gfritzsche][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/046b5f9f5822
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [good first bug][lang=JS][mentor=gfritzsche][fixed-in-fx-team] → [good first bug][lang=JS][mentor=gfritzsche]
Target Milestone: --- → Firefox 29
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: