[SeaMonkey] "TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/dom/tests/browser/browser_ConsoleStorageAPITests.js | 0 events found, tab close is clearing the cache"

VERIFIED FIXED in Firefox 11

Status

P2
major
VERIFIED FIXED
7 years ago
2 months ago

People

(Reporter: sgautherie, Assigned: sgautherie)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 13
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox10 wontfix, firefox11 verified, firefox12 verified, firefox-esr10 affected)

Details

(Whiteboard: [perma-orange], URL)

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

7 years ago
http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1329533748.1329538419.29635.gz
WINNT 5.2 comm-central-trunk debug test mochitest-other on 2012/02/17 18:55:48
+
http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey-Aurora/1329548148.1329554173.27000.gz
WINNT 5.2 comm-aurora debug test mochitest-other on 2012/02/17 22:55:48
+
http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey-Beta/1329553178.1329556949.32205.gz
Linux comm-beta debug test mochitest-other on 2012/02/18 00:19:38
{
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/dom/tests/browser/browser_ConsoleStorageAPITests.js | 0 events found, tab close is clearing the cache
}

*****

http://mxr.mozilla.org/comm-central/search?string=ConsoleAPIStorage.jsm&case=on

Is this check/test supposed to be "devtools/webconsole/HUDService.jsm" specific?
Otherwise, "how" is (SeaMonkey) Error Console supposed to clear the cache?
(Assignee)

Comment 1

7 years ago
PS: I reproduce on Windows 2000 with an Opt build.
Serge: ConsoleAPI and ConsoleAPIStorage are independent of the HUDService (Web Console). The ConsoleAPIStorage is also unrelated to the Error Console.

The dom/base/ConsoleAPI.js file implements the window.console API. While ConsoleAPIStorage.jsm is a cache for all the window.* API calls. This is used by the Web Console to display all the calls that have happened before the Web Console opens.

The storage functionality is, nonetheless, independent from the Web Console.

Why would it fail on SeaMonkey? Can you please provide more details?
(Assignee)

Comment 3

7 years ago
Created attachment 598510 [details] [diff] [review]
(Av1) browser_ConsoleStorageAPITests.js: Improve code a little
[Checked in: Comments 14 and 29]

While here...
Attachment #598510 - Flags: review?(gavin.sharp)
(Assignee)

Comment 4

7 years ago
(In reply to Mihai Sucan [:msucan] from comment #2)

> The storage functionality is, nonetheless, independent

Iiuc, this test (and ConsoleAPI and ConsoleAPIStorage) is expected to be "self-contained" in Gecko and should be independent of the built application, isn't it?

> Why would it fail on SeaMonkey?

I have no idea.

> Can you please provide more details?

I don't think I have more, except the check fails because there is 1 message instead of 0.

***

Another guess:

http://mxr.mozilla.org/mozilla-central/source/dom/base/ConsoleAPIStorage.jsm
observes "inner-window-destroyed".
Maybe SeaMonkey doesn't send it (because it doesn't destroy it?) when the tab is removed?

Hopefully, Neil should know more...
(In reply to Serge Gautherie (:sgautherie) from comment #4)
> (In reply to Mihai Sucan [:msucan] from comment #2)
> 
> > The storage functionality is, nonetheless, independent
> 
> Iiuc, this test (and ConsoleAPI and ConsoleAPIStorage) is expected to be
> "self-contained" in Gecko and should be independent of the built
> application, isn't it?

What I mean is ConsoleAPI/ConsoleAPIStorage do not depend on any Firefox browser UI, nor on any of the browser devtools code. (afaik!)

Most likely SeaMonkey is compiled with different configs that inadvertently break the ConsoleAPI/ConsoleAPIStorage code.

Comment 6

7 years ago
I didn't even realise that we had started providing the window.console API. What does it do? Are we supposed to have UI for it?

By default, when we close a tab, we send it into bfcache, which I believe doesn't actually destroy the inner window (if bfcache succeeds).

You can disable that by setting the preference browser.tabs.max_tabs_undo to 0.
(In reply to neil@parkwaycc.co.uk from comment #6)
> I didn't even realise that we had started providing the window.console API.
> What does it do? Are we supposed to have UI for it?

It lets developers log messages to the Web Console. I'm not sure what you mean by "supposed to" - it's certainly not required, but your users might find it useful (as with any of the devtools Firefox has).

> By default, when we close a tab, we send it into bfcache, which I believe
> doesn't actually destroy the inner window (if bfcache succeeds).

This would explain why the test fails in SeaMonkey. I guess this test should set that pref (is it live?).
Attachment #598510 - Flags: review?(gavin.sharp) → review+

Comment 8

7 years ago
(In reply to Gavin Sharp from comment #7)
> (In reply to comment #6)
> > I didn't even realise that we had started providing the window.console API.
> > What does it do? Are we supposed to have UI for it?
> 
> It lets developers log messages to the Web Console. I'm not sure what you
> mean by "supposed to" - it's certainly not required, but your users might
> find it useful (as with any of the devtools Firefox has).

Well, won't people think it odd that we uselessly expose window.console?

> > By default, when we close a tab, we send it into bfcache, which I believe
> > doesn't actually destroy the inner window (if bfcache succeeds).
> 
> This would explain why the test fails in SeaMonkey. I guess this test should
> set that pref (is it live?).

Not only is it live, it's actually live after-the-fact, so setting it to 0 clears all cached tabs.
(Assignee)

Comment 9

7 years ago
Created attachment 598598 [details] [diff] [review]
(Av2) browser_ConsoleStorageAPITests.js: Set "browser.tabs.max_tabs_undo" preference, Document and improve code

Av1, and more.

Key points:
*Set needed preference, to fix this bug.
*Remove tearDown(), which is better handled by the harness.
*Remove try+catch, which has no (auto-)documented purpose.
 (Let harness handle this too.)
*Add executeSoon(), to let observe() fully end, fwiw.

http://mxr.mozilla.org/comm-central/search?string=browser.tabs.max_tabs_undo&case=1&find=%2Fsuite%2F.*%2Fbrowser-prefs.js
Assignee: nobody → sgautherie.bz
Attachment #598510 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #598598 - Flags: review?(gavin.sharp)
You don't need the extra executeSoon, finish is already called at the end of the existing one.

Can you write the pref-setting patch on top of the one I already reviewed?
(Assignee)

Comment 11

7 years ago
(In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #10)

> You don't need the extra executeSoon, finish is already called at the end of
> the existing one.

Hum, actually, "browser-test.js > nextTest()" does scope cleanup since "recent"(for me) bug 658738.
This "executeSoon(finish)" trick should not be needed for browser-chrome tests anymore.
I'll remove it on checkin.

> Can you write the pref-setting patch on top of the one I already reviewed?

Interdiff seems to work on these two patches: would that be enough to review it?
(In reply to Serge Gautherie (:sgautherie) from comment #11)
> Hum, actually, "browser-test.js > nextTest()" does scope cleanup since
> "recent"(for me) bug 658738.
> This "executeSoon(finish)" trick should not be needed for browser-chrome
> tests anymore.

I don't understand how you reached that conclusion. executeSoon(finish) isn't really related to scope cleanup, its purpose is to let the stack unwind before triggering the next test. This really should have been built-in to the test harness, though... we might be able to still do that.

> Interdiff seems to work on these two patches: would that be enough to review
> it?

More trouble for me. mq lets you do this easily, can you not use it?
(Assignee)

Comment 13

7 years ago
(In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #12)

> I don't understand how you reached that conclusion. executeSoon(finish)
> isn't really related to scope cleanup, its purpose is to let the stack
> unwind before triggering the next test. This really should have been

Hum, not the same use, though it does both (somewhat), doesn't it?
Anyway, I filed bug 728638.
Stack unwind (and related) is what I had in mind when writing to check whether each use is a trick or wanted there.

> built-in to the test harness, though... we might be able to still do that.

Feel free to add a comment to bug 615546: might try to do it for browser-chrome only/first, or revive my SimpleTest and/or TestRunner patches...

> > Interdiff seems to work on these two patches: would that be enough to review
> > it?
> 
> More trouble for me. mq lets you do this easily, can you not use it?

Oh. Interdiff is built-in in Bugzilla, whereas I need to (re)create a diff patch locally.
But if you require it I'll do :-/
(Assignee)

Updated

7 years ago
Attachment #598510 - Attachment is obsolete: false
(Assignee)

Comment 14

7 years ago
Comment on attachment 598510 [details] [diff] [review]
(Av1) browser_ConsoleStorageAPITests.js: Improve code a little
[Checked in: Comments 14 and 29]

https://hg.mozilla.org/mozilla-central/rev/4d47329bb02e

[Approval Request Comment]
Regression caused by (bug #): Bug 612658.
User impact if declined: None, but will be needed to apply next/fix patch.
Testing completed (on m-c, etc.): this comment.
Risk to taking this patch (and alternatives if risky): None, test only.
String changes made by this patch: None.
Attachment #598510 - Attachment description: (Av1) browser_ConsoleStorageAPITests.js: Improve code a little → (Av1) browser_ConsoleStorageAPITests.js: Improve code a little [Checked in: Comment 14]
Attachment #598510 - Flags: approval-mozilla-beta?
Attachment #598510 - Flags: approval-mozilla-aurora?
Comment on attachment 598510 [details] [diff] [review]
(Av1) browser_ConsoleStorageAPITests.js: Improve code a little
[Checked in: Comments 14 and 29]

Please don't complicate this bug by asking for approval on separate patches. Generating a patch on top of another is simple, and you don't need to push the patch to do it. If you're not familiar with MQ I suggest you read up on it.
Attachment #598510 - Flags: approval-mozilla-beta?
Attachment #598510 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 16

7 years ago
Created attachment 598628 [details] [diff] [review]
(Bv1) browser_ConsoleStorageAPITests.js: Set "browser.tabs.max_tabs_undo" preference, Document and improve code

Av2, after pushing Av1,
and with comment 10 suggestion(s).
Attachment #598598 - Attachment is obsolete: true
Attachment #598628 - Flags: review?(gavin.sharp)
Attachment #598598 - Flags: review?(gavin.sharp)
(Assignee)

Comment 17

7 years ago
(In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #15)

> Please don't complicate this bug by asking for approval on separate patches.
> Generating a patch on top of another is simple, and you don't need to push

Please, don't give contradictory instructions:
first you refuse to review my unified patch, then you complain I'm dealing with 2 patches!

> the patch to do it. If you're not familiar with MQ I suggest you read up on
> it.

Please stop lecturing me about how mq/things is *so* easy when you seem unwilling to use interdiff/mq yourself.
Sorry for the misunderstanding. My requests weren't contradictory: I asked that you post two patches for the purposes of review, but request approval on a single patch.

If you're not familiar with writing a patch on top of another patch using MQ, I'd be happy to offer help on IRC.
Comment on attachment 598628 [details] [diff] [review]
(Bv1) browser_ConsoleStorageAPITests.js: Set "browser.tabs.max_tabs_undo" preference, Document and improve code

Let's just add these two lines to the test:

Services.prefs.setIntPref("browser.tabs.max_tabs_undo", 0);
registerCleanupFunction(function() {
  Services.prefs.clearUserPref("browser.tabs.max_tabs_undo");
});
Attachment #598628 - Flags: review?(gavin.sharp)
(Assignee)

Comment 20

7 years ago
(In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #19)
> Let's just add these two lines to the test:
> 
> Services.prefs.setIntPref("browser.tabs.max_tabs_undo", 0);
> registerCleanupFunction(function() {
>   Services.prefs.clearUserPref("browser.tabs.max_tabs_undo");
> });

Why not use SpecialPowers.pushPrefEnv(), which is meant to do that (better), iiuc?
Why not do the rest of the cleanup?
Because it isn't necessary, and has the potential to break things.
(Assignee)

Comment 22

7 years ago
Created attachment 599910 [details] [diff] [review]
(Bv2) browser_ConsoleStorageAPITests.js: Set "browser.tabs.max_tabs_undo" preference, Document code
[Checked in: Comments 25 and 28]

Bv1, with comment 19 suggestion(s).


(In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #21)
> Because it isn't necessary, and has the potential to break things.

Not required: I agree.
Potential to break: not obvious to me, but without details I can't really argue :-|
Attachment #598628 - Attachment is obsolete: true
Attachment #599910 - Flags: review?(gavin.sharp)
(Assignee)

Comment 23

7 years ago
(In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #19)
> Services.prefs.setIntPref("browser.tabs.max_tabs_undo", 0);
> registerCleanupFunction(function() {
>   Services.prefs.clearUserPref("browser.tabs.max_tabs_undo");
> });

Ftr, note that I don't understand either why you want a 2nd cleanup function.
Attachment #599910 - Flags: review?(gavin.sharp) → review+
I didn't notice that there was already a cleanup function registered. It doesn't really matter, multiple cleanup functions don't hurt.
(Assignee)

Comment 25

7 years ago
Comment on attachment 599910 [details] [diff] [review]
(Bv2) browser_ConsoleStorageAPITests.js: Set "browser.tabs.max_tabs_undo" preference, Document code
[Checked in: Comments 25 and 28]

https://hg.mozilla.org/mozilla-central/rev/cd120efbe4c6


[Approval Request Comment]
See comment 14.
Attachment #599910 - Attachment description: (Bv2) browser_ConsoleStorageAPITests.js: Set "browser.tabs.max_tabs_undo" preference, Document code → (Bv2) browser_ConsoleStorageAPITests.js: Set "browser.tabs.max_tabs_undo" preference, Document code [Checked in: Comment 25]
Attachment #599910 - Flags: approval-mozilla-beta?
Attachment #599910 - Flags: approval-mozilla-aurora?
(Assignee)

Updated

7 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
status-firefox-esr10: --- → affected
Flags: in-testsuite+
Resolution: --- → FIXED
(Assignee)

Comment 26

7 years ago
http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1330121294.1330126569.11375.gz
OS X 10.6 comm-central-trunk debug test mochitest-other on 2012/02/24 14:08:14
http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1330125193.1330132229.22777.gz
WINNT 5.2 comm-central-trunk debug test mochitest-other on 2012/02/24 15:13:13

V.Fixed
Status: RESOLVED → VERIFIED
Comment on attachment 599910 [details] [diff] [review]
(Bv2) browser_ConsoleStorageAPITests.js: Set "browser.tabs.max_tabs_undo" preference, Document code
[Checked in: Comments 25 and 28]

[Triage Comment]
Approved for Aurora 12 and Beta 11 based upon this being a test-only change.
Attachment #599910 - Flags: approval-mozilla-beta?
Attachment #599910 - Flags: approval-mozilla-beta+
Attachment #599910 - Flags: approval-mozilla-aurora?
Attachment #599910 - Flags: approval-mozilla-aurora+
(Assignee)

Updated

7 years ago
Keywords: checkin-needed
Whiteboard: [perma-orange] → [c-n: 4d47329bb02e + cd120efbe4c6 to c-a and c-b] [perma-orange]
(Assignee)

Updated

7 years ago
Whiteboard: [c-n: 4d47329bb02e + cd120efbe4c6 to c-a and c-b] [perma-orange] → [c-n: 4d47329bb02e + cd120efbe4c6 to m-a and m-b] [perma-orange]
Comment on attachment 599910 [details] [diff] [review]
(Bv2) browser_ConsoleStorageAPITests.js: Set "browser.tabs.max_tabs_undo" preference, Document code
[Checked in: Comments 25 and 28]

http://hg.mozilla.org/releases/mozilla-aurora/rev/c3be19a1a2be
http://hg.mozilla.org/releases/mozilla-beta/rev/43f98e3d359c
Attachment #599910 - Attachment description: (Bv2) browser_ConsoleStorageAPITests.js: Set "browser.tabs.max_tabs_undo" preference, Document code [Checked in: Comment 25] → (Bv2) browser_ConsoleStorageAPITests.js: Set "browser.tabs.max_tabs_undo" preference, Document code [Checked in: Comments 25 and 28]
Comment on attachment 598510 [details] [diff] [review]
(Av1) browser_ConsoleStorageAPITests.js: Improve code a little
[Checked in: Comments 14 and 29]

http://hg.mozilla.org/releases/mozilla-aurora/rev/8d2531bb41b3
http://hg.mozilla.org/releases/mozilla-beta/rev/4d136c1fece0
Attachment #598510 - Attachment description: (Av1) browser_ConsoleStorageAPITests.js: Improve code a little [Checked in: Comment 14] → (Av1) browser_ConsoleStorageAPITests.js: Improve code a little [Checked in: Comments 14 and 29]
status-firefox11: affected → fixed
status-firefox12: affected → fixed
Keywords: checkin-needed
Whiteboard: [c-n: 4d47329bb02e + cd120efbe4c6 to m-a and m-b] [perma-orange] → [perma-orange]
(Assignee)

Comment 30

7 years ago
http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey-Aurora/1330509264.1330514338.13867.gz
Linux comm-aurora debug test mochitest-other on 2012/02/29 01:54:24

http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey-Beta/1330489986.1330493973.10985.gz
OS X 10.6 comm-beta debug test mochitest-other on 2012/02/28 20:33:06

seamonkey2.9 and seamonkey2.8: verified.
status-firefox11: fixed → verified
status-firefox12: fixed → verified

Updated

2 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.