1379095 - missing nsIRequest in nsAboutCacheEntry::Channel QI

Status: RESOLVED FIXED

(Core :: Networking: Cache, defect)

Description

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

hit assertion while running netwerk/test/browser/browser_about_cache.js in debug build
>ASSERTION: QueryInterface needed: 'query_result.get() == mRawPtr'

Comment 1

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: Bug 1379095 - add nsIRequest in nsAboutCacheEntry::Channel QI.

nsAboutCacheEntry::Channel is introduced by Bug 1266196. Assert_NoQueryNeeded() check is failed
due to nsAboutCacheEntry::Channel return nullptr while QI into a nsCOMPtr<nsIRequest> pointer.

Review commit: https://reviewboard.mozilla.org/r/155174/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/155174/

Comment 2

[Honza Bambas (:mayhemer)]

Comment on attachment 8884225 [details]
Bug 1379095 - add nsIRequest in nsAboutCacheEntry::Channel QI.

https://reviewboard.mozilla.org/r/155174/#review160242

Uuups!!  Thanks!

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8e0c107d042f
add nsIRequest in nsAboutCacheEntry::Channel QI. r=mayhemer

Comment 4

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/8e0c107d042f

Comment 5

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

@mayhemer do you think this patch worth uplifting to Firefox 55 (which is beta now)?

Comment 6

[Honza Bambas (:mayhemer)]

(In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment #5)
> @mayhemer do you think this patch worth uplifting to Firefox 55 (which is
> beta now)?

Yep.

Comment 7

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8884225 [details]
Bug 1379095 - add nsIRequest in nsAboutCacheEntry::Channel QI.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1266196
[User impact if declined]: no, fix test case error
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: n/a
[Is the change risky?]: no
[Why is the change risky/not risky?]: one line fix for QueryInterface
[String changes made/needed]: no

Comment 8

[Julien Cristau [:jcristau]]

(In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment #7)
> Comment on attachment 8884225 [details]
> Bug 1379095 - add nsIRequest in nsAboutCacheEntry::Channel QI.
> 
> Approval Request Comment
> [Feature/Bug causing the regression]: bug 1266196
> [User impact if declined]: no, fix test case error

So this is an old bug with no user impact?  What makes it worth uplifting?

Comment 9

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

(In reply to Julien Cristau [:jcristau] from comment #8)
> (In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment
> #7)
> > Comment on attachment 8884225 [details]
> > Bug 1379095 - add nsIRequest in nsAboutCacheEntry::Channel QI.
> > 
> > Approval Request Comment
> > [Feature/Bug causing the regression]: bug 1266196
> > [User impact if declined]: no, fix test case error
> 
> So this is an old bug with no user impact?  What makes it worth uplifting?

To reduce the noise in test automation.

Comment 10

[Julien Cristau [:jcristau]]

Comment on attachment 8884225 [details]
Bug 1379095 - add nsIRequest in nsAboutCacheEntry::Channel QI.

fix nsAboutCacheEntry::Channel queryinterface, beta55+

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/a36197d667da

Comment 12

[Andrei Vaida [:avaida]]

(In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment #7)
> [Is this code covered by automated tests?]: yes
> [Has the fix been verified in Nightly?]: yes
> [Needs manual test from QE? If yes, steps to reproduce]: no

Setting qe-verify- based on Shih-Chiang Chien's assessment on manual testing needs and the fact that this fix has automated coverage.