Closed Bug 630284 Opened 13 years ago Closed 13 years ago

cloud9ide.com - cloud9ide breaks on nightly build (works on beta)

Categories

(Core :: JavaScript Engine, defect)

Product:
Core
Component:
JavaScript Engine
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla2.0b11
Tracking Flags:
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: dietrich, Assigned: cdleary)

References

Details

(Whiteboard: hardblocker)

Attachments

(1 file)

Add |RegExp.compile| back. Boo.
6.64 KB, patch
brendan
: review+
sayrer
: approval2.0+
Details | Diff | Splinter Review 
http://run.cloud9ide.com/

Nightly build: blank white page

Beta: see a login page
Last good nightly: 2011-01-27 First bad nightly: 2011-01-28

Pushlog: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=e0fc18b3bc
41&tochange=b5314bc1a926
Matthias, your dates looks correct but the changeset ids looks like one day before?
The regression window I get on Linux is 2011-01-27-03 -- 2011-01-28-03
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=b5314bc1a926&tochange=0e50480c408f
which has a TM merge with bug 623435 in it.

The Error Console when loading the URL:
Error: this.percentageMatch.compile is not a function
Source File: http://run.cloud9ide.com/js/apf_release.js
Line: 1

In that script we find:
        this.percentageMatch = new RegExp();
        this.percentageMatch.compile("([\\-\\d\\.]+)\\%", "g");
        this.reMatchXpath = new RegExp();
        this.reMatchXpath.compile("(^|\\|)(?!\\@|[\\w-]+::)", "g");

so I guess bug 623435 is likely the cause of the exception...

The page does not display because some script add style="display: none;" to
the <html> element.  I'm guessing some other script is supposed to later remove
that but the JS error above prevented that code from being reached.

-> Tech Evang
Assignee: nobody → english-us
Blocks: 623435
Component: General → English US
OS: Linux → All
Product: Firefox → Tech Evangelism
QA Contact: general → english-us
Hardware: x86 → All
Summary: cloud9ide breaks on nightly build (works on beta) → cloud9ide.com - cloud9ide breaks on nightly build (works on beta)
Oh crap. I thought my old compile method was a "pre-extension", something from Netscape 4 not standardized in ES3 and not implemented by other browsers. But

javascript:re=/hi/;re.compile("bye");alert(re.test("bytebye"))

alerts true in Safari and Chrome. Can someone please test IE? Whatever IE does this invalidates our thinking in ripping out compile. We may need to morph this bug from TE back to JS Engine and restore the lost compile method. It's way late to be ripping something out that other browsers implement and real web apps use.

/be
http://cloud9ide.lighthouseapp.com/projects/67519-cloud9-ide/tickets/44-regexpprototypecompile-is-non-standard-and-shouldnt-be-used

Not that I expect that's enough to say we shouldn't revert this close to release, but for actively-maintained code, there's no reason not to think they wouldn't change their code if we ask nicely.  :-)
Waldo: there's never only one cockroach (no offense to cloud9 -- the cockroach is mutable regexp instances but that will be fixed with #/.../# in Harmony).

Any IE9 results for the javascript: url in comment 4.

/be
IE9 alerts true.

Whether or not there's only one cockroach, it's still a cockroach.  If we can squash it (not now), we should try, and not give up on the first sign of failure.
That's true in general, but we shouldn't be fumigating with guests about to arrive.  Restore compatibility and fight it in the next cycle, IMO.
(In reply to comment #7)
Oops, that mixes referents a bit.  If we were happy to see /x/.compile go originally, we should still be happy to see it go if we can convince people using it not to.

I completely agree about not fumigating now.  I just don't think it's worth giving up completely, past 4.0.
Waldo: I love your diligence at roach hunting, but please think about the houseguests arriving for the Fx4 party _subito_. I never, ever, not once in this bug or anywhere said that you should not have filed a bug on cloud9.

/be
Oops again, I misread "Whatever" in comment 4 as "whether", suggesting that if IE9 implemented it that settled it for the future, past 4.0.  Sorry for that!
I don't think the logic in comment 11 is sound, but never mind. Our logic in ripping out compile was unsound too (p->q can go wrong if !p [unsound] or if p does not always imply q [invalid]). We thought no one implemented compile bug us. Boo to us for not testing.

We can leave this as an evang bug but we need a new bug to restore compile for Fx4. Chris, it can be cleaner and simpler if you redo rather than revive, I bet. Can you file?

Allen, could you please add this to the de-facto regexp standard pile to consider? Again for #/.../# it won't be a method, but depending on how much the web depends on it, it may fall into the same category as other stuff not in ES3 for mutable RegExps.

/be
IE does implement RegExp compile.  See http://msdn.microsoft.com/en-us/library/ff955265%28v=VS.85%29.aspx

I was kept in in IE9 for web compatibility...
Assignee: english-us → cdleary
Status: NEW → ASSIGNED

        Attachment #508903 -
        Flags: review?(brendan)

    
    

    
  
Comment on attachment 508903 [details] [diff] [review]
Add |RegExp.compile| back. Boo.

># HG changeset patch
># Parent f1afdc854d3fd0ca4bfb82fc4a9093ca4f33fc24
>diff --git a/js/src/jsregexp.cpp b/js/src/jsregexp.cpp
>--- a/js/src/jsregexp.cpp
>+++ b/js/src/jsregexp.cpp
>@@ -571,17 +571,17 @@ js::Class js_RegExpClass = {
>     NULL,           /* hasInstance */
>     JS_CLASS_TRACE(regexp_trace)
> };
> 
> /*
>  * RegExp instance methods.
>  */
> 
>-JSBool
>+extern JSBool

Why these externs? They're extern'ed in the .h file and we don't duplicate that in the .cpp (or in .c in the old days). And we don't even need the externs in the .h file with C++, right? Anyway, seems unnecessary to this patch and against house style, so please don't add.

r=me with this, thanks. Will mark up to get into b11 today, leaving the push to you.

/be

    
    

    
  
Now we do need a separate TE bug to track Waldo's cloud9 ticket.

/be
Component: English US → JavaScript Engine
Product: Tech Evangelism → Core
QA Contact: english-us → general
Target Milestone: --- → mozilla2.0b11

        Attachment #508903 -
        Flags: approval2.0?
blocking2.0: --- → ?

    
  

        Attachment #508903 -
        Flags: approval2.0? → approval2.0+

    
    

    
  
Comment on attachment 508903 [details] [diff] [review]
Add |RegExp.compile| back. Boo.

With Waldo' ToObject suggestion.

/be

        Attachment #508903 -
        Flags: review?(brendan) → review+
blocking2.0: ? → betaN+
Whiteboard: hardblocker

    
    

    
  
http://hg.mozilla.org/mozilla-central/rev/3568f13924d2
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Flags: in-testsuite+

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: