Closed Bug 702080 Opened 10 years ago Closed 8 years ago
Audit existing Get
Display Document() calls to find ones that should be changed to Is Resource Doc()
Bug 700895 reminded me that I (or someone) should audit all existing calls to GetDisplayDocument() -- for any of these calls that appear to be as safety-checks for external-resources-documents, we should call IsResourceDoc() instead of GetDisplayDoc(), so that we cover SVG-as-an-image, too.
Taking, so that this doesn't fall off the radar. (If someone else wants to do this and gets to it before I do, though, feel free to steal it.)
Assignee: nobody → dholbert
Hi, Just a question to see if i got it: we should call IsResourceDoc() when GetDisplayDoc() is called in a conditionnal statement without return assignation? just like here: http://mxr.mozilla.org/mozilla-central/source/content/base/public/nsContentPolicyUtils.h#293 If i'm right a can take it otherwise correct me :)
Apparently Comment 1 wasn't enough to let me keep this from falling off the radar. :) Thanks for finding & resurrecting it, Arnaud! To answer your question: it's not as straightforward as that. Some of the GetDisplayDocument() calls are for code that's about to use the display-document (if it exists), and it's just doing a null-check before proceeding. That's the case in the code that you linked to. It's kind of a case-by-case thing. The cases we're interested in are when GetDisplayDocument() is really a check for things that we don't want to allow helper-documents to do (or things that they don't need to do). *Those* are the checks where IsResourceDoc() would be appropriate. From doing a quick scan of an MXR search, the candidate GetDisplayDocument() calls that I think probably want to be converted to IsResourceDoc() are: http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsFrameLoader.cpp#301 http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsFrameLoader.cpp#1795 http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsFrameLoader.cpp#1970 http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsFrame.cpp#4886 and I think that's it. Want to try converting those ones, give it a try run, and see how it goes? (bz would know more surely than I whether those ^ cases are appropriate for switching to IsResourceDoc(); he should probably review this.)
Assigning to Arnaud, since it looks like he's interested in taking a crack at a patch for this. (but Arnaud, feel free to punt it back to me.)
Assignee: dholbert → six.dsn
The frameloaders ones, I think yes. The nsFrame one, yes if we don't rely on painting inside the resource doc directly for SVG images..
Ok thanks for the details, i will patch it during the week end should i add reftests to the try run (or any other)?
Status: NEW → ASSIGNED
Probably best to just run all the tests for this, to be on the safe side. You can keep it to 1 or 2 platforms, though (both debug + opt on each platform) -- if anything's going to break, that'd be likely to flush it out. (Unlike the MOZ_OVERRIDE bug, this definitely can & should impact behavior, albeit in edge-case ways.)
I changed the lines Daniel has indicate. Build on Linux64 Tbpl is here: https://tbpl.mozilla.org/?tree=Try&rev=20fe856b8b65
Attachment #763680 - Flags: review?(bzbarsky)
Comment on attachment 763680 [details] [diff] [review] replace GetDisplayDocument with IsResourceDoc when needed This seems fine, though some of the "are we chrome" bits that check GetDisplayDocument might really want to do something magical for svg images....
Attachment #763680 - Flags: review?(bzbarsky) → review+
Ok so are we good for checkin-needed?
Not quite checkin-needed yet -- this hasn't gotten any unit tests. The Try build in comment 8 just shows builds -- no tests. (Arnaud: You were unlucky enough to pick two of the platforms where we don't do unit tests, because they're not supported, I think. :) Or something like that. So "-u all" for those platforms did nothing.) Could you do one more Try build with, say, linux and macosx64 as the platforms, and all unit tests?
Ok i'm really unlucky... :) i will send another Try tomorrow morning
I just launched a new Try: https://tbpl.mozilla.org/?tree=Try&rev=5e8c86d0528f
Daniel it seems to be ok, can you confirm and if it's ok set checkin-needed ;)
Looks good to me!
This patch landed alongside three other patches <https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=80cf4c214910> and caused a b2g mochitest-3 test failure <https://tbpl.mozilla.org/php/getParsedLog.php?id=24315422&tree=Mozilla-Inbound>, so I backed it out, since it's the only patch that touches something b2g related: https://hg.mozilla.org/integration/mozilla-inbound/rev/1d4c00e49f93 Please watch <https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=1d4c00e49f93> to see if it makes the b2g mochitest-3 suite green again, which would prove that this patch has indeed been at fault.
Yup, this backout didn't fix it, so this was innocent. I backed out the other bug, and rather than re-landing this on a closed tree (and complicating the already-confusing bustage-upon-bustage state of the tree), I'm going to just mark it as checkin-needed so it can re-land tomorrowish.
Daniel is right. I relanded the patch: https://hg.mozilla.org/integration/mozilla-inbound/rev/42ec2b2ca987 Sorry for my mistake.
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
You need to log in before you can comment on or make changes to this bug.