Closed
Bug 957922
Opened 10 years ago
Closed 10 years ago
"aBrowser is null" in _setPluginNotificationIcon
Categories
(Firefox :: Address Bar, defect)
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)
414 bytes,
text/html
|
Details | |
1.44 KB,
patch
|
Details | Diff | Splinter Review |
JavaScript error: chrome://browser/content/browser.js, line 4192: aBrowser is null
Comment 1•10 years ago
|
||
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
Comment 2•10 years ago
|
||
gfritzsche, you interested in mentoring somebody for this?
Component: Plugin Finder Service → Location Bar
Product: Toolkit → Firefox
Whiteboard: [good first bug][lang=JS]
Comment 3•10 years ago
|
||
Sure, why not.
Whiteboard: [good first bug][lang=JS] → [good first bug][lang=JS][mentor=gfritzsche]
Comment 5•10 years ago
|
||
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?
Comment 7•10 years ago
|
||
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 9•10 years ago
|
||
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 10•10 years ago
|
||
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+
Updated•10 years ago
|
Status: NEW → ASSIGNED
Comment 11•10 years ago
|
||
Bhagya, can you do the two minor changes requested in comment 10 and put a new patch up?
Assignee | ||
Comment 12•10 years ago
|
||
Attachment #8361038 -
Attachment is obsolete: true
Attachment #8365903 -
Flags: review?(georg.fritzsche)
Comment 13•10 years ago
|
||
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+
Assignee | ||
Comment 14•10 years ago
|
||
Attachment #8365903 -
Attachment is obsolete: true
Attachment #8365924 -
Flags: checkin?(jaws)
Updated•10 years ago
|
Attachment #8365924 -
Flags: checkin?(jaws)
Updated•10 years ago
|
Keywords: checkin-needed
Comment 15•10 years ago
|
||
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]
Comment 16•10 years ago
|
||
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.
Description
•