Closed
Bug 790499
Opened 12 years ago
Closed 12 years ago
Time API: Call JS_ClearDateCaches() to update Date object's timezone when the system timezone is reset
Categories
(Core :: DOM: Device Interfaces, defect, P1)
Core
DOM: Device Interfaces
Tracking
()
People
(Reporter: airpingu, Assigned: slee)
References
Details
(Whiteboard: [LOE:M])
Attachments
(1 file, 4 obsolete files)
5.10 KB,
patch
|
slee
:
review+
|
Details | Diff | Splinter Review |
The timezone change notification must be propagated to JS Date object by calling JS_ClearDateCaches() so that the Date.getTimezoneOffset() won't get a deprecated timezone info.
Reporter | ||
Updated•12 years ago
|
Updated•12 years ago
|
blocking-basecamp: --- → ?
Assignee | ||
Updated•12 years ago
|
Assignee: nobody → slee
Updated•12 years ago
|
blocking-basecamp: ? → +
Assignee | ||
Updated•12 years ago
|
Whiteboard: [LOE:M]
Assignee | ||
Comment 1•12 years ago
|
||
nsSystemTimeChangeObserver calls JS_ClearDateCaches when timezone is changed.
TimeSetting register system time change observer for making sure that
JS_ClearDateCaches must be called in nsSystemTimeChangeObserver. It handles
the condition that nobody listens to moztimechange event and someone changes
timezone.
Attachment #660674 -
Flags: review?(justin.lebar+bug)
Comment 2•12 years ago
|
||
Comment on attachment 660674 [details] [diff] [review]
patch
We can nit about the plumbing here which calls ClearDateCaches, but first, I'd like to make sure we get ClearDateCaches() correct.
Sean, is ClearDateCaches() here correct? Or do we need to somehow iterate over all JS runtimes? (So as to catch workers? I'm really naive here.)
Attachment #660674 -
Flags: review?(sstangl)
Comment 3•12 years ago
|
||
Comment on attachment 660674 [details] [diff] [review]
patch
Clearing this from my queue until sstangl has a chance to look at this.
Attachment #660674 -
Flags: review?(justin.lebar+bug)
Comment 4•12 years ago
|
||
Comment on attachment 660674 [details] [diff] [review]
patch
Review of attachment 660674 [details] [diff] [review]:
-----------------------------------------------------------------
This is safe, assuming that there is only a single instance of the JS engine. On multiprocess, JS_ClearDateCaches() must be called for each process that runs SpiderMonkey.
Internally, JS_ClearDateCaches() just updates a global double LocalTZA. Since the double is process-global, and WebWorkers are threads within a single process, each thread will eventually see an updated LocalTZA, assuming cache coherency. However, because of races, it is not guaranteed that no incorrect timezone information can be returned after JS_ClearDateCaches() is called.
Attachment #660674 -
Flags: review?(sstangl) → review+
Updated•12 years ago
|
Attachment #660674 -
Flags: review?(justin.lebar+bug)
Comment 5•12 years ago
|
||
Comment on attachment 660674 [details] [diff] [review]
patch
I have some suggestions about how to improve the TimeSetting class. As is, I
think it's pretty confusing, because, despite how it appears at first,
TimeSetting is merely a timezone setting change observer: It watches the
settings API and notices when the timezone changes.
As far as I can tell, nobody is supposed to call TimeSetting::SetTimezone (and
indeed, nobody does call TimeSetting::SetTimezone, afaict). Instead, one
should call hal::SetTimezone.
Because this class is an observer (and only an observer):
* it should not be called "TimeSetting", which suggests that it can be
correctly used to set the current time. ("TimeSetting" is doubly
misleading, because this class notices changes in the /timezone/, not the
time, and because it's not used for setting anything.)
A better name might be TimezoneSettingObserver or TimezoneSettingWatcher.
* TimeSetting::SetTimezone() should not be a public static method, because
that suggests that it's meant to be called by other code.
One way to make very clear the fact that this class is only an observer is to
remove the class from the header file altogether. Instead, the header file
could contain only a single function, for initialization. See for example
xpcom/base/AvailableMemoryTracker.h.
You could call mozilla::system::StartWatchingTimezoneSetting() (or whatever you
decide to call it) from SystemWorkerManager, where we currently initialize the
static reference to the TimeSetting instance. You could either explicitly call
StopWatchingTimezoneSetting() on shutdown, or alternatively (and probably
better), you could register your own shutdown listener as part of
StartWatchingTimezoneSetting(). (You probably could just use ClearOnShutdown.)
The design principal at work here is that we should write interfaces such that
the correct ways of using them and the obvious ways of using them are the same.
In this case, there are a number of obvious (to me, anyway) ways to use
TimeSetting that would be incorrect. And it's not at all obvious that the
correct way to use it is by creating a static reference to an instance of the
class on startup.
I'd be happy to review any changes you'd like to make to this code (which we
should do in a separate bug).
Now that I have that out of my system, I'll do the review of this patch that I
owe you. :)
Comment 6•12 years ago
|
||
Comment on attachment 660674 [details] [diff] [review]
patch
>diff --git a/dom/system/gonk/TimeSetting.h b/dom/system/gonk/TimeSetting.h
>+#include "mozilla/Hal.h"
> #include "jspubtd.h"
> #include "nsIObserver.h"
>
> namespace mozilla {
> namespace system {
>
> class ResultListener;
>
>-class TimeSetting : public nsIObserver
>+class TimeSetting : public nsIObserver,
>+ public hal::SystemTimeObserver
> {
> public:
> NS_DECL_ISUPPORTS
> NS_DECL_NSIOBSERVER
>
> TimeSetting();
> virtual ~TimeSetting();
> static nsresult SetTimezone(const JS::Value &aValue, JSContext *aContext);
>+ void Notify(const mozilla::hal::SystemTimeChange& aReason) {}
> };
>diff --git a/dom/system/gonk/TimeSetting.cpp b/dom/system/gonk/TimeSetting.cpp
>@@ -97,16 +97,18 @@ TimeSetting::TimeSetting()
>+ hal::RegisterSystemTimeChangeObserver(this);
>@@ -124,16 +126,17 @@ nsresult TimeSetting::SetTimezone(const
> TimeSetting::~TimeSetting()
> {
> nsCOMPtr<nsIObserverService> observerService = services::GetObserverService();
> if (observerService) {
> observerService->RemoveObserver(this, MOZSETTINGS_CHANGED);
> }
>+ hal::UnregisterSystemTimeChangeObserver(this);
> }
Maybe I'm missing something, but it's not clear to me that this does what we want.
I understand that this code is to cover the case (from comment 1) "that nobody
listens to moztimechange event and someone changes timezone."
But suppose nobody is listening to the moztimechange event. That means that
TimeChangeObserver's mWindowListeners.Length() == 0, right? And when that
happens, TimeChangeObserver calls
UnregisterSystemTimeChangeObserver(sObserver).
So I don't see how calling hal::RegisterSystemTimeChangeObserver(this) above
ensures that TimeChangeObserver is notified of changes to the system time.
It seems to me that the simplest thing to do would be to have
hal::NotifySystemTimeChange() call JS_ClearDateCaches. (We should call it
whenever the time changes, not just when the timezone changes.)
But does hal::NotifySystemTimeChange get called in every process? If not, none
of this will work. It looks like hal::NotifySystemTimeChange doesn't get broadcast to all processes, but I'm not done reading quite yet...
Attachment #660674 -
Flags: review?(justin.lebar+bug) → review-
Comment 7•12 years ago
|
||
> It looks like hal::NotifySystemTimeChange doesn't get broadcast to all processes, but I'm not done
> reading quite yet...
Yeah, I don't see how this gets broadcast to all processes, so atm, it looks like NotifySystemTimeChange will only notify the main process, instead of notifying all processes as it should. So that's another bug you'll have to fix here or elsewhere.
Assignee | ||
Comment 8•12 years ago
|
||
(In reply to Justin Lebar [:jlebar] from comment #5)
> Comment on attachment 660674 [details] [diff] [review]
> I'd be happy to review any changes you'd like to make to this code (which we
> should do in a separate bug).
>
> Now that I have that out of my system, I'll do the review of this patch that
> I
> owe you. :)
Hi Justin,
I looked Bug783021. The timezone is set through settings api. TimeSetting is to
observe the timezone change. When timezone is changed, TimeSetting should call
hal::SetTimezone(). If we want to change TimeSetting to a pure observer, we may
need a way to set time zone and I am happy to take this job. :)
Comment 9•12 years ago
|
||
> If we want to change TimeSetting to a pure observer, we may
> need a way to set time zone and I am happy to take this job. :)
Could you rephrase this? I don't understand.
Assignee | ||
Comment 10•12 years ago
|
||
(In reply to Justin Lebar [:jlebar] from comment #6)
> Comment on attachment 660674 [details] [diff] [review]
> patch
> So I don't see how calling hal::RegisterSystemTimeChangeObserver(this) above
> ensures that TimeChangeObserver is notified of changes to the system time.
Ah, you are right.
>
> It seems to me that the simplest thing to do would be to have
> hal::NotifySystemTimeChange() call JS_ClearDateCaches. (We should call it
> whenever the time changes, not just when the timezone changes.)
>
> But does hal::NotifySystemTimeChange get called in every process? If not,
> none
> of this will work. It looks like hal::NotifySystemTimeChange doesn't get
> broadcast to all processes, but I'm not done reading quite yet...
My original thought was to call JS_ClearDateCaches in
hal::NotifySystemTimeChange. But I think it's a little strange
to call JS functions in hal. So that I tried to implement it
in TimeSetting. It seems that I had better to call this function
in hal::NotifySystemTimeChange.
Assignee | ||
Comment 11•12 years ago
|
||
(In reply to Justin Lebar [:jlebar] from comment #9)
> Could you rephrase this? I don't understand.
I think I misunderstood what you mean. What you want is
1. Change the name to a suitable one.
2. Hide TimeSetting class since it is not meant to be called by
others.
Is that right?
Comment 12•12 years ago
|
||
> My original thought was to call JS_ClearDateCaches in
> hal::NotifySystemTimeChange. But I think it's a little strange
> to call JS functions in hal.
I agree. If you wanted to add a separate observer just for this, that would be fine with me. It's a bit clumsy either way.
> I think I misunderstood what you mean. What you want is
> 1. Change the name to a suitable one.
> 2. Hide TimeSetting class since it is not meant to be called by
> others.
> Is that right?
Yes. I'd add maybe (3) Add some (brief!) documentation.
Assignee | ||
Comment 13•12 years ago
|
||
Add an extra observer to observe system clock/time zone change. This observer
lives in nsGlobalWindow. It should solve the problem that only main process
can be notified when time zone changes.
Attachment #660674 -
Attachment is obsolete: true
Attachment #662428 -
Flags: feedback?(justin.lebar+bug)
Comment 14•12 years ago
|
||
Comment on attachment 662428 [details] [diff] [review]
patch
This is the right idea, but it has two problems:
- We'll reset the date caches once for every window we have. nsGlobalWindow is not (despite its name) a top-level Firefox window. Rather, it corresponds to a JS "global", which corresponds to a JS |window| object. We get one nsGlobalWindow for each tab and iframe, plus more for Firefox's chrome windows.
We only want to call JS_ClearDateCaches once per process.
- Not all processes have an nsGlobalWindow instance. Most do, but in B2G, we have a "spare" process which we use to accelerate app startup, and I don't think that process has any nsGlobalWindows. But we still want to call JS_ClearDateCaches in that process when the system time changes.
If you want to keep the JS_ClearDateCaches() call out of hal, perhaps it would make sense to incorporate this code into the time zone change code we talked about elsewhere. Somewhere, you call mozilla::time::InitializeTimeChangeObservers(), and that creates a time zone change observer (if appropriate -- we don't want one on all systems) and also creates a time change observer, like above.
Attachment #662428 -
Flags: feedback?(justin.lebar+bug) → feedback-
Assignee | ||
Comment 15•12 years ago
|
||
Attachment #662428 -
Attachment is obsolete: true
Attachment #662863 -
Flags: review?(justin.lebar+bug)
Comment 16•12 years ago
|
||
Comment on attachment 662863 [details] [diff] [review]
patch
>diff --git a/dom/system/gonk/TimeSetting.cpp b/dom/system/gonk/TimeSetting.cpp
>--- a/dom/system/gonk/TimeSetting.cpp
>+++ b/dom/system/gonk/TimeSetting.cpp
> do_GetService("@mozilla.org/settingsService;1");
> if (!settingsService) {
> ERR("Failed to get settingsLock service!");
> return;
> }
> settingsService->CreateLock(getter_AddRefs(lock));
> nsCOMPtr<nsISettingsServiceCallback> callback = new InitTimezoneCb();
> lock->Get(TIME_TIMEZONE, callback);
>+
>+ time::InitializeDateCacheCleaner();
> }
Hmm. This file is Gonk-only code. In theory, we want to have a
DateCacheCleaner on all platforms, because in theory, we should inform the
JSAPI when the system timezone changes on any platform.
In practice, we're not /listening/ for timezone changes on any other platform.
But I feel like we still ought to run this code everywhere, so that if and when
we do start listening for timezone changes on other platforms, we won't wonder
why JS isn't hearing those changes.
How about we just put this in nsLayoutStatics::Initialize()?
(That has the added advantage of always being called, whereas TimeSetting() is
called from SystemWorkerManager() which is lazily initialized, so probably has
the same problem in the "spare" process as we had before.)
>diff --git a/dom/time/DateCacheCleaner.h b/dom/time/DateCacheCleaner.h
>new file mode 100644
>--- /dev/null
>+++ b/dom/time/DateCacheCleaner.h
>@@ -0,0 +1,10 @@
Please add a brief comment here explaining what InitializeDateCacheCleaner()
does.
>+namespace mozilla {
>+namespace time {
>+void InitializeDateCacheCleaner();
>+} //namespace time
>+} //namespace mozilla
>diff --git a/dom/time/DateCacheCleaner.cpp b/dom/time/DateCacheCleaner.cpp
>new file mode 100644
>--- /dev/null
>+++ b/dom/time/DateCacheCleaner.cpp
>@@ -0,0 +1,59 @@
>+#include "mozilla/Hal.h"
>+#include "mozilla/ClearOnShutdown.h"
>+#include "DateCacheCleaner.h"
>+
>+#include "nsIJSContextStack.h"
>+#include "mozilla/StaticPtr.h"
>+
>+
>+using namespace mozilla::hal;
>+
>+typedef mozilla::Observer<mozilla::hal::SystemTimeChange> SystemTimeChangeObserver;
Since this isn't a header file, we prefer |using namespace| to explicit
namespace names:
using namespace mozilla;
using namespace mozilla::hal;
typedef Observer<SystemTimeChange> SystemTimeChangeObserver;
Although it seems to me that a more correct way to do this would be to move the
typedef into hal/HalTypes.h (like SwitchObserver) and then to include
hal/HalTypes.h here and in the one other place SystemTimeChangeObserver is
used.
>+namespace mozilla {
>+namespace time {
>+
>+class DateCacheCleaner : public SystemTimeChangeObserver
>+{
>+public:
>+ DateCacheCleaner()
>+ {
>+ RegisterSystemTimeChangeObserver(this);
>+ }
>+
>+ ~DateCacheCleaner()
>+ {
>+ UnregisterSystemTimeChangeObserver(this);
>+ }
>+ void Notify(const SystemTimeChange& aReason)
Nit: Newline after }.
>+ {
>+ nsCOMPtr<nsIThreadJSContextStack> stack =
>+ do_GetService("@mozilla.org/js/xpc/ContextStack;1");
>+ if (!stack) {
>+ NS_WARNING("Failed to get JSContextStack");
>+ }
>+ JSContext *cx = stack->GetSafeJSContext();
>+ if (!cx) {
>+ NS_WARNING("Failed to GetSafeJSContext");
>+ }
>+ JS_ClearDateCaches(cx);
Sorry, I was completely wrong -- we only want to do this when the timezone
changes, as you had it originally. (I mean, it's not hurting anything how it
is, but the only thing impacted by the ClearDateCaches call is the JS engine's
timezone cache.)
Would you mind putting back the check you had originally?
This is basically right, but I'd like to take another look.
Attachment #662863 -
Flags: review?(justin.lebar+bug) → review-
Assignee | ||
Comment 17•12 years ago
|
||
(In reply to Justin Lebar [:jlebar] from comment #16)
> How about we just put this in nsLayoutStatics::Initialize()?
>
> (That has the added advantage of always being called, whereas TimeSetting()
> is
> called from SystemWorkerManager() which is lazily initialized, so probably
> has
> the same problem in the "spare" process as we had before.)
This is a better place to call InitializeDateCacheCleaner. I wanted to find
such place but I was not sure where is good.
> Sorry, I was completely wrong -- we only want to do this when the timezone
> changes, as you had it originally. (I mean, it's not hurting anything how it
> is, but the only thing impacted by the ClearDateCaches call is the JS
> engine's
> timezone cache.)
>
> Would you mind putting back the check you had originally?
That's OK, just put the code back.
Assignee | ||
Comment 18•12 years ago
|
||
Attachment #662863 -
Attachment is obsolete: true
Attachment #663299 -
Flags: review?(justin.lebar+bug)
Comment 19•12 years ago
|
||
Comment on attachment 663299 [details] [diff] [review]
patch - V2
It looks like you missed one of the nits from earlier
> Nit: Newline after }.
But I'll add that before I push this.
Thanks for the patch!
Attachment #663299 -
Flags: review?(justin.lebar+bug) → review+
Comment 20•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/9f16bba541cc
(Note: I pushed this without try coverage, because I think it has a better-than-even chance of going green.)
Comment 21•12 years ago
|
||
Backed out because of build bustage: https://hg.mozilla.org/integration/mozilla-inbound/rev/57deb0de3513
Assignee | ||
Comment 22•12 years ago
|
||
Hi Justin,
Sorry for the convenience.
I found that one crash problem is resulted from that both
sDateCacheCleaner and sSystemTimeObservers are deleted by
ClearOnShutdown mechanism.
The initial sequence is sDateCacheCleaner then sSystemTimeObservers.
And release order is sSystemTimeObservers then sDateCacheCleaner.
It looks good. But when deleting sDateCacheCleaner, it calls
UnregisterSystemChangeObserver and this function try to use
sSytemTimeObservers. Then the program crashes. The patch of
Bug 792443 changes the usage of sSystemTimeObservers which does
not use StaticAutoPtr. I will try it on try server and check
whether it can solve the current problem.
Comment 23•12 years ago
|
||
Sounds good to me.
Assignee | ||
Comment 24•12 years ago
|
||
Hi Justin,
After Bug792243 is applied, I found the test cases still crashed. The cause of the
problem is that when the chrome process is killed or the ipc between chrome process
or content process is disconnected. The ipc resource of content process is freed
and content process tries to shutdown. It calls UnregisterSystemChangeObserver and
uses the ipc resource that is freed. After discussed with Cervantes, we found that
HalParent implements ActorDestroy to handle such condition but HalChild does not.
http://mxr.mozilla.org/mozilla-central/source/hal/sandbox/SandboxHal.cpp#381
We think we should have a ActorDestroy for HalChild to deal with it.
I will file another bug to solve this problem first.
Assignee | ||
Comment 25•12 years ago
|
||
Hi Justin,
The crash problem is resulted from the release sequence of content process. For detail information, please check https://bugzilla.mozilla.org/show_bug.cgi?id=793759#c10.
In comment, https://bugzilla.mozilla.org/show_bug.cgi?id=793759#c11, cjones said that we should not unregister content process's observers on shutdown. If we don't unregister observers, we get many memory leak reports in test cases, https://tbpl.mozilla.org/?tree=Try&rev=f2b896453307 :(. I think I can
1. do not unregister observers on content process. There will be memory leak but only when the content process is going down.
2. call the destructor during uiMessageLoop.MessageLoop::Run()[1]. I need some time to check where is the proper places to call.
[1]http://mxr.mozilla.org/mozilla-central/source/toolkit/xre/nsEmbedFunctions.cpp#485
Comment 26•12 years ago
|
||
> If we don't unregister observers, we get many memory leak reports in test cases,
> https://tbpl.mozilla.org/?tree=Try&rev=f2b896453307 :(.
Are you certain those are content-process leaks? I'm not sure our infrastructure even does leak checking in the content process, but even if it did, I would be surprised if we started a content process on all desktop test suites.
Assignee | ||
Comment 27•12 years ago
|
||
(In reply to Justin Lebar [:jlebar] from comment #26)
> Are you certain those are content-process leaks? I'm not sure our
> infrastructure even does leak checking in the content process, but even if
> it did, I would be surprised if we started a content process on all desktop
> test suites.
Yes, I did the test again and found that the leak happens in content process.
Comment 28•12 years ago
|
||
If it's really content process leaks that are turning the test suite orange, then you obviously can't follow cjones's advice to allow those objects to leak.
But could you show me how you know that they're content process leaks? I'm pretty surprised that we load a content process, for example, on Windows crashtests (one of tests which turned orange in the try build from comment 25).
Assignee | ||
Comment 29•12 years ago
|
||
(In reply to Justin Lebar [:jlebar] from comment #28)
> But could you show me how you know that they're content process leaks? I'm
> pretty surprised that we load a content process, for example, on Windows
> crashtests (one of tests which turned orange in the try build from comment
> 25).
I log the process's type by |XRE_GetProcessType()| and its process id. After test cases are done, I compare the process type and pid with my log. I found that the leaking processes are content processes.
Comment 30•12 years ago
|
||
I do not think this is a feature. By not fixing this bug, an application showing a date (like the status bar) will not be correctly updated if the user decides to change the timezone of his device IIRC.
... but in my current B2G build, it's not possible to change the time (or it's very well hidden) so I can't confirm that.
Reporter | ||
Comment 31•12 years ago
|
||
(In reply to Mounir Lamouri (:mounir) from comment #30)
> I do not think this is a feature. By not fixing this bug, an application
> showing a date (like the status bar) will not be correctly updated if the
> user decides to change the timezone of his device IIRC.
>
> ... but in my current B2G build, it's not possible to change the time (or
> it's very well hidden) so I can't confirm that.
Just leaving some notes here if anyone is hoping to change system time:
1. Time clock: navigator.mozTime.set((new Date()));
2. Time zone: navigator.mozSettings.createLock({time.timezone: "America/Chicago"});
Assignee | ||
Comment 32•12 years ago
|
||
(In reply to Mounir Lamouri (:mounir) from comment #30)
> I do not think this is a feature. By not fixing this bug, an application
> showing a date (like the status bar) will not be correctly updated if the
> user decides to change the timezone of his device IIRC.
>
> ... but in my current B2G build, it's not possible to change the time (or
> it's very well hidden) so I can't confirm that.
IanLiu is working on this feature. You can change the date and time on Settings app now.
Comment 33•12 years ago
|
||
(In reply to StevenLee from comment #32)
> (In reply to Mounir Lamouri (:mounir) from comment #30)
> > I do not think this is a feature. By not fixing this bug, an application
> > showing a date (like the status bar) will not be correctly updated if the
> > user decides to change the timezone of his device IIRC.
> >
> > ... but in my current B2G build, it's not possible to change the time (or
> > it's very well hidden) so I can't confirm that.
>
> IanLiu is working on this feature. You can change the date and time on
> Settings app now.
I've heard it has landed a few hours ago.
Comment 34•12 years ago
|
||
After talking with Steven, we agreed with Mounir that it's not a feature implementation case. I removed 'feature' from keywords. Tag back if it's needed. Suggestions and/or comments are welcome. Thanks.
Keywords: feature
Comment 35•12 years ago
|
||
FYI, I landed bug 796523 to clear the timezone cache whenever we create a new compartment.
I don't think that obviates this bug, or makes it not a blocker, particularly because we need to update the timezone cache /immediately/ so that the clock displays the correct time, and triggering that update on gc / compartment creation just won't work.
Updated•12 years ago
|
Priority: -- → P1
Comment 36•12 years ago
|
||
Steven, did you made any progress regarding the issues you found (re comment 25)?
Assignee | ||
Comment 37•12 years ago
|
||
(In reply to Mounir Lamouri (:mounir) from comment #36)
> Steven, did you made any progress regarding the issues you found (re comment
> 25)?
Hi Mounir,
Bug793759 is landed that fixed the memory leaking problem. The testing results on try server confuses me. At first, the test cases crashed on WinXP-debug, https://tbpl.mozilla.org/?tree=Try&rev=f21fcf0e1a98. I tried to build and tested on windows. But a few days later, I updated the source code and tried again. The crash problem is gone. Then the same condition happens on Linux64-debug https://tbpl.mozilla.org/?tree=Try&rev=f6411f383a6c. I tried to figure out the problem. Today I updated the source code and tried the same patch on try server again. There is no crash problem on Linux64-debug but it has crash problem on OS X 10.7 debug. I am not sure if I could check in the patch now. :(
Assignee | ||
Comment 38•12 years ago
|
||
rebase
Attachment #663299 -
Attachment is obsolete: true
Attachment #672197 -
Flags: review+
Comment 39•12 years ago
|
||
Try run for bbb73a4579fa is complete.
Detailed breakdown of the results available here:
https://tbpl.mozilla.org/?tree=Try&rev=bbb73a4579fa
Results (out of 237 total builds):
success: 219
warnings: 18
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/slee@mozilla.com-bbb73a4579fa
Assignee | ||
Updated•12 years ago
|
Keywords: checkin-needed
Comment 40•12 years ago
|
||
> I am not sure if I could check in the patch now. :(
You can always ask on IRC for help interpreting tryserver results. edmorley and philor can definitely help, and others probably can as well.
I pushed this to inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/80fffd499dca
If you remind me tomorrow, I can push this to Aurora for you.
Updated•12 years ago
|
Keywords: checkin-needed
Comment 41•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/80fffd499dca
Should this have a test?
Status: NEW → RESOLVED
Closed: 12 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Comment 42•12 years ago
|
||
status-firefox18:
--- → fixed
status-firefox19:
--- → fixed
Does the fix here remove the need to have bug 815414 fixed?
You need to log in
before you can comment on or make changes to this bug.
Description
•