Closed Bug 1407229 Opened 3 years ago Closed 3 years ago

Port bug 1406278 to C-C: Changed parameters of HTMLImageElement::SetSrc/GetSrc()

Categories

(Thunderbird :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 58.0

People

(Reporter: jorgk-bmo, Assigned: jorgk-bmo)

Details

Attachments

(1 file, 3 obsolete files)

c:/builds/moz2_slave/tb-c-cen-w64-ntly-000000000000/build/mailnews/compose/src/nsMsgCompose.cpp(274): error C2660: 'mozilla::dom::HTMLImageElement::GetSrc': function does not take 1 arguments [log…]
c:/builds/moz2_slave/tb-c-cen-w64-ntly-000000000000/build/mailnews/compose/src/nsMsgCompose.cpp(445): error C2660: 'mozilla::dom::HTMLImageElement::SetSrc': function does not take 2 arguments [log…]
Boris, what's the magic we need to apply here?

This patch gives two compile errors:
'nsIPrincipal': cannot instantiate abstract class
'void mozilla::dom::HTMLImageElement::SetSrc(const nsAString &,nsIPrincipal &,nsINode::ErrorResult &)': cannot convert argument 2 from 'nullptr' to 'nsIPrincipal &'

Oh, I see, Get/SetHref have changed, too.
Assignee: nobody → jorgk
Status: NEW → ASSIGNED
Attachment #8916983 - Flags: feedback?(bzbarsky)
OK, by digging around in the (generated) HTMLImageElementBinding.cpp I found something that compiles.

Boris, can you please advise further.
Attachment #8916983 - Attachment is obsolete: true
Attachment #8916983 - Flags: feedback?(bzbarsky)
Attachment #8917011 - Flags: feedback?(bzbarsky)
Comment on attachment 8917011 [details] [diff] [review]
1407229-setsrc.patch - WIP - compiles but isn't right

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

::: mailnews/compose/src/nsMsgSend.cpp
@@ +1890,2 @@
>      } else if (image) {
> +      image->SetSrc(NS_ConvertASCIItoUTF16(domSaveArray[i].url), subjectPrincipal, rv2);

Looks like everywhere else in the patch we do a Get call populating 'subjectPrincipal' before doing a Set call. This is the only place without the Get call. Should I do a Get call just to retrieve the principal?
We should add overloads of the relevant functions without the principal argument on the Gecko side.  The fact that Get* was affected at all is just because bindings are a little silly.

For SetSrc on image we should also add a no-principal overload, that looks like this:

  void SetSrc(const nsAString& aSrc, ErrorResult& aError)
  {
    SetHTMLAttr(nsGkAtoms::src, aSrc, aError);
  }

Similar for SetHref on <link>.
Comment on attachment 8917011 [details] [diff] [review]
1407229-setsrc.patch - WIP - compiles but isn't right

Passing through uninitialized NonNull is not ok.  It will pass a random value, and probably end up crashing.
Attachment #8917011 - Flags: feedback?(bzbarsky) → feedback-
Attached patch 1407229-setsrc.patch (v2) (obsolete) — Splinter Review
Temporary fix.

As discussed with Boris and Kris (John-Galt?) on IRC, I can use the     NonNull<nsIPrincipal> subjectPrincipal; for the get calls and a system principal for the set calls.
Attachment #8917011 - Attachment is obsolete: true
The NonNull<nsIPrincipal> subjectPrincipal; actually asserts, so that's not an option. So I came up with another hack, since I didn't want to use *(nsIPrincipal*)nullptr.

OK, I'll land this now to unbust TB. How's going to add the overloads, as per IRC:
18:28:14 - bz: Anyway, it's clear how to fix this: add some non-principal-taking overloads
Attachment #8917042 - Attachment is obsolete: true
Flags: needinfo?(kmaglione+bmo)
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/2ca8d8d56941
Temporary fix for changes in bug 1406278: Changed parameters of HTMLImageElement::SetSrc/GetSrc() and HTMLLinkElement::SetHref(). rs=bustage-fix
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
(In reply to Jorg K (GMT+2) from comment #8)
> The NonNull<nsIPrincipal> subjectPrincipal; actually asserts, so that's not
> an option.

That's good to hear. I was going to file a bug if it didn't.

>  So I came up with another hack, since I didn't want to use
> *(nsIPrincipal*)nullptr.

Is there a particular reason not to just pass the system principal, as bz
suggested?

> OK, I'll land this now to unbust TB. How's going to add the overloads, as
> per IRC:
> 18:28:14 - bz: Anyway, it's clear how to fix this: add some
> non-principal-taking overloads

I'll have patches for that sometime this morning.
Flags: needinfo?(kmaglione+bmo)
(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #10)
> Is there a particular reason not to just pass the system principal, as bz
> suggested?
Apparently a misunderstanding. I wanted to avoid passing the real value of the system principal as a reference to a function that could possibly overwrite it. Well, we know that the callee wouldn't have looked at it.

This will bug be backed out 100% once bug 1407334 lands.
Keywords: leave-open
Status: RESOLVED → REOPENED
Keywords: leave-open
Resolution: FIXED → ---
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/dd2c759074f4
Backed out changeset 2ca8d8d56941: Backout of temporary fix. a=jorgk
Status: REOPENED → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → FIXED
We're done here since bug 1407334 landed on M-C minutes before the backout in comment #12. So the hacky stuff never got shipped in a Daily, but it helped to keep the tree open and do other stuff in the meantime.

Boris and Kris, thanks again for your help.
Target Milestone: --- → Thunderbird 58.0
You need to log in before you can comment on or make changes to this bug.