Last Comment Bug 794067 - Sync Feed Preview with Firefox as at 2012/09
: Sync Feed Preview with Firefox as at 2012/09
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: Feed Discovery and Preview (show other bugs)
: Trunk
: All All
: -- normal (vote)
: seamonkey2.15
Assigned To: Philip Chee
:
Mentors:
: 767919 (view as bug list)
Depends on: 809783
Blocks:
  Show dependency treegraph
 
Reported: 2012-09-25 06:50 PDT by Philip Chee
Modified: 2012-12-14 08:45 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
fixed
fixed
fixed


Attachments
Patch v1.0 Sync changes. (9.63 KB, patch)
2012-09-25 06:54 PDT, Philip Chee
iann_bugzilla: review+
Details | Diff | Splinter Review
Patch v2.0 Branch safe patch. (8.51 KB, patch)
2012-10-01 09:23 PDT, Philip Chee
philip.chee: review+
bugspam.Callek: approval‑comm‑aurora+
bugspam.Callek: approval‑comm‑beta+
Details | Diff | Splinter Review

Description Philip Chee 2012-09-25 06:50:50 PDT
To Do.
Comment 1 Philip Chee 2012-09-25 06:54:14 PDT
Created attachment 664488 [details] [diff] [review]
Patch v1.0 Sync changes.

Changes:

FeedConverter.js:
Bug 714631 Feed converter should check HTTP channel response status before proceeding.
Bug 783738 Use Components.results.NS_BINDING_ABORTED instead of hardcoding the value.

WebContentConverter.js:
Bug 520374 WebContentConverter.js needlessly includes debug.js. [NS_ASSERT]

nsFeedSniffer.h:
Bug 758992 Make the classes which use the XPCOM nsISupports implementation macros final, to avoid the warning about deleting using a pointer to a base class with virtual functions and no virtual dtor (browser parts)

nsFeedSniffer.cpp:
Bug 739040 Crash in nsStringBuffer::Realloc() during Feed sniffing (on second visit ?)
Bug 739040 Use the length of the decoded string, if no more than 512 bytes, to sniff for feeds.
Bug 739040 followup: properly enforce the sniffing limit on uncompressed feeds. Plus tests.

$ TEST_PATH=suite/browser/test/  pymake -C ../objdir-sm/ mochitest-plain

67 INFO TEST-START | Shutdown
68 INFO Passed: 54
69 INFO Failed: 0
70 INFO Todo:   2
71 INFO SimpleTest FINISHED
72 INFO TEST-INFO | Ran 0 Loops
73 INFO SimpleTest FINISHED

Note: excludes context menu tests in the same directory.
Comment 2 Ian Neal 2012-09-30 03:30:57 PDT
(In reply to Philip Chee from comment #1)
> Created attachment 664488 [details] [diff] [review]
> Patch v1.0 Sync changes.
> 
> Changes:
> 
> Bug 783738 Use Components.results.NS_BINDING_ABORTED instead of hardcoding
> the value.
Should this not cover changes to nsBrowserContentHandler.js, nsBrowserStatusHandler.js and uploadProgress.js too? There is EdLinkChecker.js too but that is in shared code (with TB) and is wider than just NS_BINDING_ABORTED so warrants a separate bug.
Comment 3 Ian Neal 2012-09-30 03:32:28 PDT
Comment on attachment 664488 [details] [diff] [review]
Patch v1.0 Sync changes.

r=me with previously mentioned NS_BINDING_ABORTED changes made.
Comment 4 Philip Chee 2012-09-30 13:19:13 PDT
> r=me with previously mentioned NS_BINDING_ABORTED changes made.
I'll do this in a separate bug with a more informative summary.
Comment 5 Philip Chee 2012-10-01 03:51:57 PDT
Pushed to comm-central:
http://hg.mozilla.org/comm-central/rev/a72e2158740a
Comment 6 Philip Chee 2012-10-01 09:23:57 PDT
Created attachment 666581 [details] [diff] [review]
Patch v2.0 Branch safe patch.

[Approval Request Comment]
Regression caused by: N/A
User impact if declined: Crashes in nsStringBuffer::Realloc() during Feed sniffing plus other stuff discussed over IRC.
Testing completed : comm-central, plus has tests. 
Risk to taking this patch (and alternatives if risky): probably none. Ports patches that have been baked on Firefox since Mozilla14.
String changes made by this patch: None.
Comment 7 Justin Wood (:Callek) (Away until Aug 29) 2012-10-02 19:41:59 PDT
Comment on attachment 666581 [details] [diff] [review]
Patch v2.0 Branch safe patch.

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

http://hg.mozilla.org/releases/comm-beta/rev/f4d39ec8e3cd
Comment 8 Philip Chee 2012-10-03 09:11:13 PDT
Pushed:
http://hg.mozilla.org/releases/comm-aurora/rev/5a9c180c8930
Comment 9 Philip Chee 2012-10-03 09:54:28 PDT
>> r=me with previously mentioned NS_BINDING_ABORTED changes made.
> I'll do this in a separate bug with a more informative summary.

I filed Bug 797410 to cover this. I also marked this a GFB.
Comment 10 Jens Hatlak (:InvisibleSmiley) 2012-11-07 13:43:11 PST
I guess the below mochitest-5 failure stems from this bug. I think test_maxSniffing needs to be turned into a chrome test (like the FF original) to fix the permission problem:

WINNT 5.2 comm-central-trunk debug test mochitests-5/5 on 2012/11/07 10:04:43
s: sea-win32-04
3775 ERROR TEST-UNEXPECTED-FAIL | /tests/suite/browser/test/test_maxSniffing.html | an unexpected uncaught JS exception reported through window.onerror - Error: Permission denied to access property 'documentElement' at http://mochi.test:8888/tests/suite/browser/test/test_maxSniffing.html:27

I don't have a working build right now but will look into this within the next few days (unless someone reading this gets to it faster). In its own bug of course, depend on this one and to be pushed to everything up to/including at least beta. Once it's fixed, mochitest-5 should be green again. :-)
Comment 11 Philip Chee 2012-12-14 08:45:12 PST
*** Bug 767919 has been marked as a duplicate of this bug. ***

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